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

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

* Pedro Alves <palves@redhat.com> [2019-01-15 23:43:07 +0000]:

> On 01/15/2019 11:03 PM, Tom Tromey wrote:
> >>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> > 
> > Andrew> Maybe there's some other reason why scoped_finish_thread_state is
> > Andrew> different, in which case I apologise for wasting everyone's time, but
> > Andrew> right now it appears to me that scoped_finish_thread_state is no
> > Andrew> different to cleanup_function, it's just used more.
> > 
> > FWIW I don't think it's a waste of time at all.  There's no particular
> > rush for these patches and I think it's more valuable for us to agree on
> > what we'd like the result to look like than it is to land them quickly.
> 
> Definitely agreed.  Not a waste at all!
> 
> I've been playing with this today, and I have a different
> implementation of Andrew's class that allows writing:
> 
>  using delete_longjmp_breakpoint_cleanup
>    = forward_cleanup_function<decltype (delete_longjmp_breakpoint),
>                               delete_longjmp_breakpoint>;
> 
> Or, with a macro to eliminate redundancy:
> 
>  using delete_longjmp_breakpoint_cleanup
>    = FORWARD_CLEANUP_FUNCTION (delete_longjmp_breakpoint);
> 
> Naming up in the air, I just picked that as straw man.
> 
> > 
> > Andrew> I think if we're going to put in a generic solution (which I think is
> > Andrew> a good thing) then we should either, make sure we understand why
> > Andrew> scoped_finish_thread_state is different (and what the rules are for
> > Andrew> when to use the generic, and when to create a class), or, make sure
> > Andrew> the generic is suitable to replace scoped_finish_thread_state.
> > 
> > Andrew> (I'm not trying to pick on scoped_finish_thread_state, it was just the
> > Andrew> first example I found when originally replying to Tom.)
> > 
> > Maybe I was just making too big a deal out of it, but my thinking was
> > that writing the finish_thread_state call at each spot would be bad,
> > since it would be multiple copies of the same thing.  But, maybe it is
> > actually no big deal?
> > 
> > Using a template, as Pedro suggested, would remove some of the ugliness
> > from the series.  Stuff like this:
> > 
> > +  auto do_invalidate
> > +    = [=] ()
> > +      {
> > +	this->invalidate (regnum);
> > +      };
> > +  cleanup_function invalidator (do_invalidate);
> > 
> > Could instead just be:
> > 
> >   SCOPE_EXIT { this->invalidate (regnum); }
> > 
> > ... assuming we like SCOPE_EXIT (to me it seems reasonable enough).
> > 
> > Anyway, I tend to think we should simply copy the scope_exit paper.  If
> > it's accepted into C++ then someday we can just remove the gdb variant.
> > 
> > Let me know if you agree; if so I can implement this.
> > 
> I've also played with the template idea, basically implemented
> scope_exit / make_scope_exit.  Seems to work nicely.
> 
> I hadn't done the SCOPE_EXIT macro though, not sure it is worth
> it to have yet another way to write these things (thinking about
> newcomers' cognitive load, having to learn all the different
> things) -- of all the make_scope_exit calls I have, most either
> take the form:
> 
>   auto cleanup = make_scope_exit (function);
> 
> i.e., are passed a function pointer, can do without a
> lambda, and/or need access to the scope_exit object to
> cancel it.  But I can give it a try for sure.  It may
> be clearer code to standardize writing:
> 
>   auto cleanup = make_scope_exit (function);
>   cleanup.release ();
> 
> when the cleanup may need to be canceled and
> 
>   SCOPE_EXIT { function (); }
> 
> when it doesn't?
> 
> I won't be able to finish this today (I'd like to clean up a couple hacks
> here and there), but I'll post something tomorrow so we can all see
> and decide a way forward.

I'm happy with either approach so long as it opens the door for
removing cleanup classes that are just calling a global function.

Thanks for all the work on this.

Thanks,
Andrew