Re: [PATCH] gdb: Replace make_bpstat_clear_actions_cleanup

[Tom Tromey <tom at tromey dot com>]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I haven't tried the branch, but I did take a look.  I've had similar
Andrew> thoughts for a while (that we could have a generic object that called
Andrew> the existing cleanup functions) but I always figured we preferred more
Andrew> specialised classes as that was what seemed to be getting submitted...

Andrew> ...personally I don't see any problem with having a generic cleanup
Andrew> object for cases where a single function call is needed.  As you point
Andrew> out in the other mail in this thread, I did pull the cleanup function
Andrew> into the cleanup object in my patch, but as the cleanup here is just a
Andrew> series of calls to global functions (there's no significant state in
Andrew> the cleanup class) I don't see that as a huge advantage.

Andrew> Did you see extending the work in your branch to remove existing
Andrew> specialised cleanup objects where possible?  I suspect many could be
Andrew> replaced with your generic approach, but one concrete example (only
Andrew> picked because it was the first I found) is gdbthread.c:class
Andrew> scoped_finish_thread_state.

First, thanks for your (off-list) review.  You found the bug.

For the record the bug was something like

   gdb::optional<mumble> cleanup;
   ...
   cleanup.reset ();

... where what was meant was:

   cleanup->reset ();

I renamed the cleanup_function's method to "cancel" to try to avoid this
problem in the future.

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.

I did replace regcache_invalidator, on the theory that since there was
only a single use, it was no big deal to introduce a new lambda.

Tom