Re: [patch] Create cleanups.[ch]

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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 :-).

> 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.

> +#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.

> +
> +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 prefer comments in the declaration, FWIW.

> +
> +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.

> +}
> +
> +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