Re: [PATCH] gdb: Replace make_bpstat_clear_actions_cleanup

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Tom Tromey <tom@tromey.com> [2019-01-07 08:27:08 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> Replace make_bpstat_clear_actions_cleanup with a new class that can
> Andrew> perform the cleanup.
> 
> Hah, I took a stab at this too.  I started with something like your
> patch but generalized it to a cleanup_function that takes a
> function_view as a callback.  This allowed for the removal of some more
> cleanups.  See my branch submit/cleanup/longjmp-bp.
> 
> I wonder what you think of that approach.  I was on the fence about it
> myself.  On the one hand, it's pretty simple to understand and it fits a
> number of existing spots in gdb.  On the other hand, maybe it's a bit
> too easy to get into trouble with something generic.
> 
> If you try the branch, be warned it still has some regressions, which is
> why I haven't submitted it yet.

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

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

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

Thanks,
Andrew