Re: [patch] Create cleanups.[ch]

[Doug Evans <dje@google.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.