Re: [RFA 09/22] Remove make_cleanup_restore_current_ui

[Trevor Saunders <tbsaunde@tbsaunde.org>]

On Thu, Oct 13, 2016 at 11:16:26AM +0100, Pedro Alves wrote:
> On 10/13/2016 07:11 AM, Eli Zaretskii wrote:
> >> Cc: Simon Marchi <simon.marchi@polymtl.ca>, gdb-patches@sourceware.org
> >> From: Pedro Alves <palves@redhat.com>
> >> Date: Thu, 13 Oct 2016 02:28:44 +0100
> >>
> >> Yeah, sounds right.  I was trying get the gdb::unique_ptr in, in order
> >> to unblock the cases I suggested you use unique_ptr, but I'm a bit
> >> confused on what to do about it now...  I _think_ people are generally
> >> OK with it.  There was some opposition, but I'm not sure anymore
> >> whether it still exists.  C++11 is now on the table, but maybe a
> >> staged approach (enable C++11 while supporting C++03 too for a while,
> >> to catch issues) would make sense anyway.
> > 
> > I still think we shouldn't have workarounds for versions of the
> > standard older than what we want to support.
> 
> That is not what the patch does...  
> 
> #1 - Starting from the position that gdb wants to build with C++03
>      compilers.
> 
> #2 - The patch adds a new smart pointer type.  In C++03, you're pretty
>      much guaranteed to need to do that.  The one the standard gives you
>      is too limited.
> 
> #3 - (owning) smart pointers all look the same for most client code - they're
>      meant to look like raw pointers afterall - i.e., ptr->foo, and *ptr
>      works as expected, transparently.
> 
> #4 - where owning smart pointer APIs tend to differ is in code around pointee
>      ownership.
> 
>      I chose to model the API on std::unique_ptr (C++11).
>      There are advantages.
> 
> #5 - We could stop here.  Done.
> 
> #6 - But I went a step further, and made it so that if we have C++11
>      handy, then we can make use of the compiler to catch bugs at
>      compile time, all while keeping the nice API.
> 
> So again:
> 
> > I still think we shouldn't have workarounds for versions of the
> > standard older than what we want to support.
> 
> there's no workaround for versions older than what we want to support
> at all.  There's the code for the version that we had decided earlier
> that we wanted to support, plus extra API enforcement when built
> with a compiler that supports a version _newer_ than what we want
> to support.
> 
> 
> I'll give you more examples of why I think forbidding C++11 completely
> is short sighted.  C++11 has a nice feature where you can tell the compiler
> that some class is "final" (not possible to to inherit from that class).
> This allows the compiler to optimize the code better.
> 
> Another nice C++11 feature is that it's possible to mark class methods
> as overriding a virtual method in the base class.  This is quite handy,
> because with C++03, if you change the method's prototype in base class,
> you have to remember to update all users.  And without the "override" marker,
> the compiler won't help you find them at all.  The program builds, and then
> fails at run time.  Maybe only after release do you notice it.
> With the override marker, the compiler now errors out pointing at all
> the places you need to fix.  Bug found at compiler time.  Wonderful.
> 
> So if we have a C++11 compiler handy, why not make use of these
> features to help with development?  I don't see how it makes
> sense to not take advantage of compiler help if available.

And infact gdb already does the same exact sort of thing in the form of
the ATTRIBUTE_PRINTF macros.

> Here's GCC conditionally using these features if available as well:
> 
>   https://gcc.gnu.org/ml/jit/2016-q2/msg00000.html
> 
> And GCC still supports building with a C++03 compiler.  Just
> like GDB.  Tons of projects out there do the same thing.

as far as conditional compilation goes this seems a lot saner than  many
of the ifdefs in gcc, and probably the rest of the toolchain.  Its also
a lot easier to test than many of them.

> > IOW, if we decide to use
> > C++11, we should _require_ a compiler that supports it, and error out
> > if the configure test(s) regarding that fail.
> > 
> 
> So:
> 
> #1.1 - for people with new compilers, we'd go through C++11 std::unique_ptr
>        all the time.
> 
> #1.2 - people with older compilers would be out of luck.  tough.  get newer
>        compiler first.
> 
> Vs:
> 
> #2.1 - for people with new compilers, we'd go through std::unique_ptr all
>        the time.
> 
> #2.2 - for people with older compilers we go through the fallback
>        implementation.  gdb still works.
> 
> (I wasn't planning on starting the discussion on whether 1.2 is
> reasonable nowadays.  That happened because someone else brought it up.)
> 
> 
> So for people with newer compilers, the result is the same.
> (#1.1 == #1.2).  While I'm proposing a solution that gives us all the
> benefits (#2.1) while keeping support for older compilers
> for a little while longer (#2.2).
> 
> > Supporting more than one standard means we will have rarely used code
> > that is almost never tested, and that means maintenance burden 
> 
> That same argument could be applied to gnulib.  Or the many HAVE_FOO's
> in the code base.  We rely on gnulib's reimplementation of the whole
> printf family of functions if the compiler doesn't support it properly
> for example!

Claiming the C++98 code will be rarely tested is only true if very few
people try to compile gdb somewhere that doesn't have a C++11 compiler,
in which case yes we might as well drop support for C++98 compilers
because supporting them doesn't help anyone.  Basically you can't have
it both ways, either there are people who benefit from support for C++98
compilers, and so they'll test C++98 only code, or those people don't
exist, and you might as well require C++11.

> The sane way to make sure the fallback code still works is
> via autotesting with the buildbot.

and doing that is a whole lot easier than say testing if gdb works
without HAVE_PERSONALITY defined since testing the latter requires some
old kernel and probably glibc.

> > that means maintenance burden 
> 
> For whom?  _I'm_ willing to maintain this code.  It's not rocket
> science code.  And then users of the smart pointer don't really
> have to care.  Whether C++11 or C++03, you write the same code.
> You may miss a gdb::move call with C++03.  No big deal.  The fix
> is just to add a gdb::move call.
> 
> Several others have said they think this is a good idea.  I've even
> ran this idea through the libstdc++ maintainer in person at the Cauldron.
> Trevor said he wants to do this in gcc as well.
> 
> If redirecting to C++11 std::unique_ptr turns out to cause trouble, or
> I'm hit by a bus, then it's trivial to remove that redirection.  All it
> takes is remove a handful of lines of code guarded with
> #if __cplusplus >= 201103 to always go through the fallback implementation.
> 
> So there's no "point of no return here".
> 
> > which
> > threaten to undo at least some part of the benefits you aspire to
> > achieve by moving to C++ in general and to a new enough C++ standard
> > in particular.
> 
> I don't understand what this is trying to say.
> 
> I propose to move forward with my plan, and if it does cause
> trouble, then we drop the gdb::unique_ptr -> std::unique_ptr mapping.
> 
> It may happen, I don't know.  We will know if it will indeed cause
> trouble in practice if we try.

having delt with very similar smart pointers in other projects that
couldn't require C++11 I'll be suprised if it causes problems.

> And _in_ parallel, as a separate discussion, we can discuss dropping
> support for C++03 and going C++11 fully.  I think we could pull it
> off, but I wouldn't want to do it in a rush, and without an easy
> fallback plan available.  The patch I sent yesterday to make use
> of C++11 with gcc >= 4.8 would be the first step.  Then if that doesn't
> cause problems, we can tell the autoconf macro to require C++11
> with a single line change.  If _requiring_ C++11 causes significant
> trouble on people, and we want to go back to supporting older compilers,
> then it's easy to revert that, and go back to the unique_ptr C++03
> fallback implementation.  This is header-only template code, so we
> really can't tell if this will cause trouble for people without
> adding actual _uses_ of the smart pointer.

Yeah, I really don't see what the down side to adding the fall back, and
then deleting it in several weeks or months or whatever it ends up
taking to require C++11 is, and I don't see what the advantage of
rushing a switch to C++11 instead is.

Trev