Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2019-01-14 20:37:41 +0000]:

> On 01/12/2019 11:50 AM, Andrew Burgess wrote:
> > I've been thinking about a similar idea for a while too.  I wondered
> > if there was a way we could make use of templates to generate some of
> > the common boiler plate cleanups.
> > 
> > This mini-series changes the first 4 of your patches to you this idea
> > so you can see how it might work.
> > 
> > First, the ideal, description, a new templated class
> > `cleanup_function` that allows you to do something like this:
> > 
> >     void
> >     delete_longjmp_breakpoint (int arg)
> >     {
> >       /* Blah, blah, blah...  */
> >     }
> > 
> >     using longjmp_breakpoint_cleanup
> >         = cleanup_function <delete_longjmp_breakpoint, int>;
> 
> It seems unnecessary to pass in the types of the arguments of
> delete_longjmp_breakpoint.  Couldn't those be extracted from
> delete_longjmp_breakpoint's type?  See below.
> 
> > 
> > This creates a new cleanup class `longjmp_breakpoint_cleanup` than can
> > then be used like this:
> > 
> >     longjmp_breakpoint_cleanup obj (thread);
> 
> I think this results in an inferior cleanup_function solution, because this
> way you can't pass in a bespoke small lambda on the spot.  I.e. you're
> forced to create a cleanup with a named function -- right?

Thanks for taking the time to provide this feedback.  I think what you
are saying aligns with Tom's final position, that passing a function
pointer or lambda to the cleanup_function constructor is better.

My motivation for bringing this patch set up at all was to address
Tom's comment in this email:

   https://www.sourceware.org/ml/gdb-patches/2019-01/msg00156.html

Where he said:

    I looked at replacing scoped_finish_thread_state, but that one
    seemed reasonable to keep as is, to me, because it is used in
    several spots, and I didn't want to repeat the similar lambdas
    everywhere.

By passing the function pointer into the type creation I had hoped to
address this concern.

Maybe there's some other reason why scoped_finish_thread_state is
different, in which case I apologise for wasting everyone's time, but
right now it appears to me that scoped_finish_thread_state is no
different to cleanup_function, it's just used more.

I think if we're going to put in a generic solution (which I think is
a good thing) then we should either, make sure we understand why
scoped_finish_thread_state is different (and what the rules are for
when to use the generic, and when to create a class), or, make sure
the generic is suitable to replace scoped_finish_thread_state.

(I'm not trying to pick on scoped_finish_thread_state, it was just the
first example I found when originally replying to Tom.)

Thanks,
Andrew


> 
> If it's the lambda itself you don't like, I think it should be
> possible to add make cleanup_function's ctor have a std::bind-like interface [1],
> so that you'd pass cleanup_function's ctor the function and arguments:
> 
>   template <typename F, typename... Args>
>   cleanup_function (F &&func, Args &&...args);
> 
> so you'd create the cleanup like:
> 
>   cleanup_function cleanup (delete_longjmp_breakpoint, thread);
> 
> Since Tromey's cleanup_function is not a template, to implement such
> a ctor, it would have to rely on std::function (or something like it) for
> type erasure, which might heap allocate if the resulting callable is large
> enough.  The advantage would be that with that you can create a cleanup_function
> without passing specifying the called function's type.  The disadvantage is
> the cost of the std::function type erasure / potential heap allocation, of course.
> 
> But we could avoid the cost/heap if we make cleanup_function a template,
> as  Andrew's version is, but I have to say that I don't really like that
> version's way of declaring the cleanup_function typedef:
> 
>  +/* Cleanup class that calls delete_longjmp_breakpoint.  */
>  +#ifdef __cpp_template_auto
>  +using longjmp_breakpoint_cleanup
>  +	= cleanup_function <delete_longjmp_breakpoint, int>;
>  +#else
>  +using longjmp_breakpoint_cleanup
>  += cleanup_function <void (*) (int), delete_longjmp_breakpoint, int>;
>  +#endif
> 
> I think it should be possible to code cleanup_function's template
> such that you could instantiate it like:
> 
>   cleanup_function<void (int)>
> 
> similarly to gdb::function_view?
> 
> That doesn't tie cleanup_function to the actual function called, just
> its type, but I wouldn't see that as a disadvantage, given this way this
> works with all sorts of callables, including lambdas.
> 
> Now, ctors don't deduce template parameter types until C++17, so with
> that template interface and C++11 we wouldn't be able to just write:
> 
>   cleanup_function cleanup (delete_longjmp_breakpoint, 0);
> 
> But, that is fixable with a helper make_cleanup_function, which would
> have us write:
> 
>   auto cleanup = make_cleanup_function (delete_longjmp_breakpoint, 0);
> 
> For the optional cleanup case, we'd need to somehow spell out the
> type, no way around it, but that's not too horrible with that interface,
> IMO:
>   cleanup_function<void (int)> cleanup;
> or:
>   cleanup_function<decltype (delete_longjmp_breakpoint)> cleanup;
> 
> (and of course a typedef could put the function's type away from view)
> 
> I'm not really sure we need a std::bind-like interface though.  I'd
> be super fine with the implementation simplicity of having to pass a
> lambda, like in scope_exit:
> 
>   auto cleanup
>     = make_scope_exit ([] { delete_longjmp_breakpoint (0); });
>   ... 
>   cleanup.release ();
> 
> It's simpler to implement, because then all you need for the
> template parameter type is a generic callable:
> 
>  template <typename Callable>
>  class scope_exit
>  ...
> 
> Note that with Alexandrescu's scope_exit (see
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf), if you
> don't need to cancel the cleanup, you can write:
> 
>  SCOPE_EXIT { delete_longjmp_breakpoint (0); };
> 
> which arguably looks cleaner.  Some people prefer avoiding macros
> that "extend" the C++ language like that, though, I think.
> 
> [1] https://en.cppreference.com/w/cpp/utility/functional/bind
> 
> Thanks,
> Pedro Alves