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

From: Pedro Alves <palves@redhat.com>

  On 01/15/2019 11:43 PM, Pedro Alves wrote:
> 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.

See patch below.  Since cleanup_function turned into scope_exit,
this new class became forward_scope_exit, because it's a "forwarding"
version of scope_exit?  I'm really not that sure about that name...
wrap_scope_exit came to mind too.  Or maybe call it cleanup_function?

This version builds on top of std::bind, which eliminates the
need for manually storing the args in the tuple and the recursive
call_function_on_tuple template code.  As shown above, it also
doesn't require manually passing the wrapped-function's parameter
types to the template, they're extracted automatically.

> 
>>
>> 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?

Yeah, I think that writing

 SCOPE_EXIT { finish_thread_state (ptid); };

in the multiple places wouldn't be a big deal.

However, I found that Andrew's idea is most useful for the
gdb::optional case, since in that case we need the cleanup's type
upfront.  The finish_thread_state cleanup is in that situation,
so I converted it to a forward_scope_exit.

>>
>> 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).

In this particular case, it can't be SCOPE_EXIT because we need
to be able to cancel the scope, i.e., we need to scope_exit's name.
So we end up with:

  auto invalidator
    = make_scope_exit ([&] { this->invalidate (regnum); });

I guess we should add an alternative macro like to be used like:

  auto invalidator 
   = NAMED_SCOPE_EXIT { this->invalidate (regnum); };

I didn't do that though.  Doesn't save that much?

Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS
in C++11 instead...

>>
>> 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 did this more fully today, following the description in the paper.

I think this is the first time I had found a use for a function-try-block,
see scope_exit ctor...

> 
> 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 did this.

> 
> 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 remembered struct on_exit I had added to gdbarch-selftests.c,
thinking of a generic SCOPE_EXIT at the time.  :-)  So I converted
that too now.

This revealed that the ESC macro in common/preprocessor.h conflicts
with a macro of the same name in readline.h...  That should go in
a separate patch.

Since both scope_exit and forward_scope_exit share some
functionality, I put the shared code/structure in
a common base class, using CRTP.

Here is patch with everything together.  WDYT?

From c616fefa268155eea243dedfb6ff54e133ee33f9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 16 Jan 2019 20:45:52 +0000
Subject: [PATCH] scope_exit

---
 gdb/breakpoint.c                |  27 ++---
 gdb/common/forward-scope-exit.h | 118 +++++++++++++++++++++
 gdb/common/preprocessor.h       |   2 +-
 gdb/common/scope-exit.h         | 185 ++++++++++++++++++++++++++++++++
 gdb/common/valid-expr.h         |  18 ++--
 gdb/gdbarch-selftests.c         |   8 +-
 gdb/gdbthread.h                 |  28 +----
 gdb/infcall.c                   |  13 +--
 gdb/infcmd.c                    |  13 +--
 gdb/inferior.h                  |   4 +-
 gdb/infrun.c                    | 230 ++++++++++++++++++----------------------
 gdb/linux-nat.c                 |  18 +---
 gdb/regcache.c                  |  34 +-----
 gdb/symfile.c                   |  27 +++--
 gdb/top.c                       |   8 +-
 gdb/ui-out.h                    |   8 +-
 gdb/utils.c                     |  17 ---
 gdb/utils.h                     |   1 -
 18 files changed, 463 insertions(+), 296 deletions(-)
 create mode 100644 gdb/common/forward-scope-exit.h
 create mode 100644 gdb/common/scope-exit.h

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

Commit Message

Comments

Patch