Re: [patch] Create cleanups.[ch]

[Doug Evans <dje at google dot com>]

Hi.  I didn't want it to seem like I ignored you. :-)

Comments inline.

On Sun, Apr 15, 2012 at 10:00 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Sunday, April 15 2012, Doug Evans wrote:
>
>> Hi.
>
> Hi Doug.
>
>> Since cleanups are a big enough source of issues, I want to separate them out.
>> This patch moves the core cleanup API into its own file.
>> It makes no other changes.
>> I have at least one more patch to go, but I want to get this done
>> first.
>
> Thanks, I'm always in favor of such API separations. ?Sorry for
> nitpicking, I know you are just moving the code around, but since you
> touched it I felt I should take a look even if it's old code (maybe,
> *especially* because of that!).
>
>> 2012-04-15 ?Doug Evans ?<dje@sebabeach.org>
>>
>> ? ? ? * cleanups.h: New file.
>> ? ? ? * cleanups.c: New file.
>> ? ? ? * Makefile.in (SFILES): Add cleanups.c
>> ? ? ? (HFILES_NO_SRCDIR): Add cleanups.h
>> ? ? ? (COMMON_OBS): Add cleanups.o
>> ? ? ? * defs.h (struct cleanup): Moved to cleanups.h
>> ? ? ? (do_cleanups,do_final_cleanups): Ditto.
>> ? ? ? (discard_cleanups,discard_final_cleanups): Ditto
>> ? ? ? (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto.
>> ? ? ? (save_cleanups,save_final_cleanups): Ditto.
>> ? ? ? (restore_cleanups,restore_final_cleanups): Ditto.
>> ? ? ? (null_cleanup): Ditto.
>> ? ? ? (make_my_cleanup,make_my_cleanup2): Delete.
>> ? ? ? (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): Delete.
>> ? ? ? * utils.c (cleanup_chain,final_cleanup_chain): Moved to cleanups.c.
>> ? ? ? (do_cleanups,do_final_cleanups): Ditto.
>> ? ? ? (discard_cleanups,discard_final_cleanups): Ditto
>> ? ? ? (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto.
>> ? ? ? (save_cleanups,save_final_cleanups): Ditto.
>> ? ? ? (restore_cleanups,restore_final_cleanups): Ditto.
>> ? ? ? (null_cleanup): Ditto.
>> ? ? ? (make_my_cleanup,make_my_cleanup2): Ditto, and make static.
>> ? ? ? All uses rewritten to use proper interface.
>> ? ? ? (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): Ditto.
>
> I remember using `Ditto' once, and being told that I should use
> `Likewise' instead. ?Anyway, I'm just bringing this because I never know
> what rule to follow :-).

I'm not aware of a specific preference.
I probably use either depending on the lunar phase or whatever :-),
and there's precedent for both.
"Go with the flow." works well on so many levels.

>> Index: cleanups.c
>> ===================================================================
>> RCS file: cleanups.c
>> diff -N cleanups.c
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ cleanups.c ? ? ? ?15 Apr 2012 19:23:28 -0000
>> @@ -0,0 +1,210 @@
>> +/* Cleanup routines for GDB, the GNU debugger.
>> +
>> + ? Copyright (C) 1986, 1988-2012 Free Software Foundation, Inc.
>
> It should be:
>
> Copyright (C) 2012 Free Software Foundation, Inc.
>
> AFAIK, since it's a new file.

I'm of two minds on this, but here I just "went with the flow."

>> +#include "defs.h"
>> +
>> +/* Chain of cleanup actions established with make_cleanup,
>> + ? to be executed if an error happens. ?*/
>> +
>> +/* Cleaned up after a failed command. ?*/
>> +static struct cleanup *cleanup_chain;
>> +
>> +/* Cleaned up when gdb exits. ?*/
>> +static struct cleanup *final_cleanup_chain;
>> +
>> +static struct cleanup *
>> +make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
>> + ? ? ? ? ? ? ? void *arg, ?void (*free_arg) (void *))
>> +{
>> + ?struct cleanup *new
>> + ? ?= (struct cleanup *) xmalloc (sizeof (struct cleanup));
>> + ?struct cleanup *old_chain = *pmy_chain;
>> +
>> + ?new->next = *pmy_chain;
>> + ?new->function = function;
>> + ?new->free_arg = free_arg;
>> + ?new->arg = arg;
>> + ?*pmy_chain = new;
>> +
>> + ?return old_chain;
>> +}
>
> Maybe this function should be named `make_my_cleanup_1', just like other
> cases in GDB? ?I think it should also have a comment here, since it's
> static and its declaration/definition is here.

Such changes run afoul of the code movement rule. :-)
For comments, I went back and added the missing ones though.

>> +
>> +static struct cleanup *
>> +make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
>> + ? ? ? ? ? ? ?void *arg)
>> +{
>> + ?return make_my_cleanup2 (pmy_chain, function, arg, NULL);
>> +}
>
> Comment for this as well. ?Same thing for all static functions.
>
>> +
>> +/* Add a new cleanup to the cleanup_chain,
>> + ? and return the previous chain pointer
>> + ? to be passed later to do_cleanups or discard_cleanups.
>> + ? Args are FUNCTION to clean up with, and ARG to pass to it. ?*/
>
> Maybe these comments can be made to fill more than 42 characters in the
> line?
>
> I also never know what's the best/recommended practice: to put the
> comments above the function's declaration (in this case, in the
> cleanups.h file), or to put comments above the function definition (as
> you did). ?Maybe someone more experienced can clarify.

I'm not sure there's a documented preference.

> I prefer comments in the declaration, FWIW.

Me too, though not enough to actually follow it religiously.

>> +
>> +struct cleanup *
>> +make_cleanup (make_cleanup_ftype *function, void *arg)
>> +{
>> + ?return make_my_cleanup (&cleanup_chain, function, arg);
>> +}
>> +
>> +/* Same as make_cleanup except also includes TDOR, a destructor to free ARG.
>> + ? DTOR is invoked when the cleanup is performed or when it is discarded. ?*/
>> +
>> +struct cleanup *
>> +make_cleanup_dtor (make_cleanup_ftype *function, void *arg,
>> + ? ? ? ? ? ? ? ?void (*dtor) (void *))
>> +{
>> + ?return make_my_cleanup2 (&cleanup_chain,
>> + ? ? ? ? ? ? ? ? ? ? ? ?function, arg, dtor);
>
> No need to break the line, I think.

Code movement violation. :-)
It's easy enough to do a later pass to clean such things up.

>> +}
>> +
>> +static void
>> +do_my_cleanups (struct cleanup **pmy_chain,
>> + ? ? ? ? ? ? struct cleanup *old_chain)
>> +{
>> + ?struct cleanup *ptr;
>> +
>> + ?while ((ptr = *pmy_chain) != old_chain)
>> + ? ?{
>> + ? ? ?*pmy_chain = ptr->next; ? ? ? ?/* Do this first in case of recursion. ?*/
>> + ? ? ?(*ptr->function) (ptr->arg);
>> + ? ? ?if (ptr->free_arg)
>> + ? ? (*ptr->free_arg) (ptr->arg);
>> + ? ? ?xfree (ptr);
>> + ? ?}
>> +}
>
> Comment above the function. ?Same thing for static functions below.
>
>> Index: cleanups.h
>> ===================================================================
>> RCS file: cleanups.h
>> diff -N cleanups.h
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ cleanups.h ? ? ? ?15 Apr 2012 19:23:28 -0000
>> @@ -0,0 +1,86 @@
>> +/* Cleanups.
>> + ? Copyright (C) 1986, 1988-2005, 2007-2012 Free Software Foundation, Inc.
>
> Copyright should be 2012.
>
> --
> Sergio

Cheers.