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

[Pedro Alves <palves@redhat.com>]

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.

Thanks,
Pedro Alves