This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 00/12] remove some cleanups using a cleanup function
* 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