Re: [RFA 09/22] Remove make_cleanup_restore_current_ui

Trevor Saunders
2016-10-13T14:26:25Z

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