Re: [patch] Create cleanups.[ch]

Re: [patch] Create cleanups.[ch]

[Doug Evans]

I wrote:

> 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.
> [...]
> +/* WARNING: The result of the "make cleanup" routines is not the intuitive
> +   choice of being a handle on the just-created cleanup.  Instead it is an
> +   opaque handle of the cleanup mechanism and represents all cleanups created
> +   from that point onwards.
> +   The result is guaranteed to be non-NULL though.  */

Heh, I'm getting ahead of myself. :-)
Ignore the last sentence there.
The wording of the previous sentence is fine with me, but one may
prefer it to say something along the lines of being the handle of
the previous cleanup ("old_chain") or something like that, or say
nothing at all other than being an opaque handle.

[patch] Create cleanups.[ch]

[Doug Evans]

Hi.

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.

Regression tested on amd64-linux.

I will check this in in a few days if there are no objections.

Note: While one might want to also move things like
make_cleanup_unpush_target into cleanups.c, I would rather not.
Functions like this are not part of the core cleanup API, they just use it.
OTOH, more generic functions like make_cleanup_restore_integer are generic
enough utilities that I'd be ok if they got moved over (akin to null_cleanup).
But I have not done so in this patch.

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.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.1199
diff -u -p -r1.1199 Makefile.in
--- Makefile.in	14 Apr 2012 05:24:53 -0000	1.1199
+++ Makefile.in	15 Apr 2012 19:23:28 -0000
@@ -690,7 +690,7 @@ SFILES = ada-exp.y ada-lang.c ada-typepr
 	bfd-target.c \
 	block.c blockframe.c breakpoint.c buildsym.c \
 	c-exp.y c-lang.c c-typeprint.c c-valprint.c \
-	charset.c cli-out.c coffread.c coff-pe-read.c \
+	charset.c cleanups.c cli-out.c coffread.c coff-pe-read.c \
 	complaints.c completer.c continuations.c corefile.c corelow.c \
 	cp-abi.c cp-support.c cp-namespace.c cp-valprint.c \
 	d-lang.c d-valprint.c \
@@ -783,7 +783,7 @@ objfiles.h vec.h disasm.h mips-tdep.h se
 gdb_curses.h bfd-target.h memattr.h inferior.h ax.h dummy-frame.h \
 inflow.h fbsd-nat.h ia64-libunwind-tdep.h completer.h inf-ttrace.h \
 solib-target.h gdb_vfork.h alpha-tdep.h dwarf2expr.h \
-m2-lang.h stack.h charset.h addrmap.h command.h solist.h source.h \
+m2-lang.h stack.h charset.h cleanups.h addrmap.h command.h solist.h source.h \
 target.h prologue-value.h cp-abi.h tui/tui-hooks.h tui/tui.h \
 tui/tui-file.h tui/tui-command.h tui/tui-disasm.h tui/tui-wingeneral.h \
 tui/tui-windata.h tui/tui-data.h tui/tui-win.h tui/tui-stack.h \
@@ -858,7 +858,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $
 	auxv.o \
 	agent.o \
 	bfd-target.o \
-	blockframe.o breakpoint.o findvar.o regcache.o \
+	blockframe.o breakpoint.o findvar.o regcache.o cleanups.o \
 	charset.o continuations.o corelow.o disasm.o dummy-frame.o dfp.o \
 	source.o value.o eval.o valops.o valarith.o valprint.o printcmd.o \
 	block.o symtab.o psymtab.o symfile.o symmisc.o linespec.o dictionary.o \
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.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#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;
+}
+
+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);
+}
+
+/* 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.  */
+
+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);
+}
+
+/* Same as make_cleanup except the cleanup is added to final_cleanup_chain.  */
+
+struct cleanup *
+make_final_cleanup (make_cleanup_ftype *function, void *arg)
+{
+  return make_my_cleanup (&final_cleanup_chain, function, arg);
+}
+
+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);
+    }
+}
+
+/* Discard cleanups and do the actions they describe
+   until we get back to the point OLD_CHAIN in the cleanup_chain.  */
+
+void
+do_cleanups (struct cleanup *old_chain)
+{
+  do_my_cleanups (&cleanup_chain, old_chain);
+}
+
+/* Discard cleanups and do the actions they describe
+   until we get back to the point OLD_CHAIN in the final_cleanup_chain.  */
+
+void
+do_final_cleanups (struct cleanup *old_chain)
+{
+  do_my_cleanups (&final_cleanup_chain, old_chain);
+}
+
+static void
+discard_my_cleanups (struct cleanup **pmy_chain,
+		     struct cleanup *old_chain)
+{
+  struct cleanup *ptr;
+
+  while ((ptr = *pmy_chain) != old_chain)
+    {
+      *pmy_chain = ptr->next;
+      if (ptr->free_arg)
+	(*ptr->free_arg) (ptr->arg);
+      xfree (ptr);
+    }
+}
+
+/* Discard cleanups, not doing the actions they describe,
+   until we get back to the point OLD_CHAIN in the cleanup chain.  */
+
+void
+discard_cleanups (struct cleanup *old_chain)
+{
+  discard_my_cleanups (&cleanup_chain, old_chain);
+}
+
+/* Discard final cleanups, not doing the actions they describe,
+   until we get back to the point OLD_CHAIN in the final cleanup chain.  */
+
+void
+discard_final_cleanups (struct cleanup *old_chain)
+{
+  discard_my_cleanups (&final_cleanup_chain, old_chain);
+}
+
+static struct cleanup *
+save_my_cleanups (struct cleanup **pmy_chain)
+{
+  struct cleanup *old_chain = *pmy_chain;
+
+  *pmy_chain = 0;
+  return old_chain;
+}
+
+/* Set the cleanup_chain to 0, and return the old cleanup_chain.  */
+
+struct cleanup *
+save_cleanups (void)
+{
+  return save_my_cleanups (&cleanup_chain);
+}
+
+/* Set the final_cleanup_chain to 0, and return the old
+   final_cleanup_chain.  */
+
+struct cleanup *
+save_final_cleanups (void)
+{
+  return save_my_cleanups (&final_cleanup_chain);
+}
+
+static void
+restore_my_cleanups (struct cleanup **pmy_chain, struct cleanup *chain)
+{
+  *pmy_chain = chain;
+}
+
+/* Restore the cleanup chain from a previously saved chain.  */
+
+void
+restore_cleanups (struct cleanup *chain)
+{
+  restore_my_cleanups (&cleanup_chain, chain);
+}
+
+/* Restore the final cleanup chain from a previously saved chain.  */
+
+void
+restore_final_cleanups (struct cleanup *chain)
+{
+  restore_my_cleanups (&final_cleanup_chain, chain);
+}
+
+/* Provide a known function that does nothing, to use as a base for
+   a possibly long chain of cleanups.  This is useful where we
+   use the cleanup chain for handling normal cleanups as well as dealing
+   with cleanups that need to be done as a result of a call to error().
+   In such cases, we may not be certain where the first cleanup is, unless
+   we have a do-nothing one to always use as the base.  */
+
+void
+null_cleanup (void *arg)
+{
+}
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.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef CLEANUPS_H
+#define CLEANUPS_H
+
+/* The cleanup list records things that have to be undone
+   if an error happens (descriptors to be closed, memory to be freed, etc.)
+   Each link in the chain records a function to call and an
+   argument to give it.
+
+   Use make_cleanup to add an element to the cleanup chain.
+   Use do_cleanups to do all cleanup actions back to a given
+   point in the chain.  Use discard_cleanups to remove cleanups
+   from the chain back to a given point, not doing them.
+
+   If the argument is pointer to allocated memory, then you need
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
+
+struct cleanup
+  {
+    struct cleanup *next;
+    void (*function) (void *);
+    void (*free_arg) (void *);
+    void *arg;
+  };
+
+/* NOTE: cagney/2000-03-04: This typedef is strictly for the
+   make_cleanup function declarations below.  Do not use this typedef
+   as a cast when passing functions into the make_cleanup() code.
+   Instead either use a bounce function or add a wrapper function.
+   Calling a f(char*) function with f(void*) is non-portable.  */
+typedef void (make_cleanup_ftype) (void *);
+
+/* WARNING: The result of the "make cleanup" routines is not the intuitive
+   choice of being a handle on the just-created cleanup.  Instead it is an
+   opaque handle of the cleanup mechanism and represents all cleanups created
+   from that point onwards.
+   The result is guaranteed to be non-NULL though.  */
+
+extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);
+
+extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
+					  void (*dtor) (void *));
+
+extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
+
+/* A special value to pass to do_cleanups and do_final_cleanups
+   to tell them to do all cleanups.  */
+#define	ALL_CLEANUPS	((struct cleanup *)0)
+
+extern void do_cleanups (struct cleanup *);
+extern void do_final_cleanups (struct cleanup *);
+
+extern void discard_cleanups (struct cleanup *);
+extern void discard_final_cleanups (struct cleanup *);
+
+extern struct cleanup *save_cleanups (void);
+extern struct cleanup *save_final_cleanups (void);
+
+extern void restore_cleanups (struct cleanup *);
+extern void restore_final_cleanups (struct cleanup *);
+
+/* A no-op cleanup.
+   This is useful when you want to establish a known reference point
+   to pass to do_cleanups.  */
+extern void null_cleanup (void *);
+
+#endif /* CLEANUPS_H */
Index: defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.311
diff -u -p -r1.311 defs.h
--- defs.h	5 Mar 2012 11:33:35 -0000	1.311
+++ defs.h	15 Apr 2012 19:23:28 -0000
@@ -251,29 +251,6 @@ enum return_value_convention
   RETURN_VALUE_ABI_PRESERVES_ADDRESS,
 };
 
-/* the cleanup list records things that have to be undone
-   if an error happens (descriptors to be closed, memory to be freed, etc.)
-   Each link in the chain records a function to call and an
-   argument to give it.
-
-   Use make_cleanup to add an element to the cleanup chain.
-   Use do_cleanups to do all cleanup actions back to a given
-   point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.
-
-   If the argument is pointer to allocated memory, then you need
-   to additionally set the 'free_arg' member to a function that will
-   free that memory.  This function will be called both when the cleanup
-   is executed and when it's discarded.  */
-
-struct cleanup
-  {
-    struct cleanup *next;
-    void (*function) (void *);
-    void (*free_arg) (void *);
-    void *arg;
-  };
-
 /* vec.h-style vectors of strings want a typedef for char * or const char *.  */
 
 typedef char * char_ptr;
@@ -313,26 +290,9 @@ extern void set_display_time (int);
 
 extern void set_display_space (int);
 
-#define	ALL_CLEANUPS	((struct cleanup *)0)
-
-extern void do_cleanups (struct cleanup *);
-extern void do_final_cleanups (struct cleanup *);
-
-extern void discard_cleanups (struct cleanup *);
-extern void discard_final_cleanups (struct cleanup *);
-extern void discard_my_cleanups (struct cleanup **, struct cleanup *);
-
-/* NOTE: cagney/2000-03-04: This typedef is strictly for the
-   make_cleanup function declarations below.  Do not use this typedef
-   as a cast when passing functions into the make_cleanup() code.
-   Instead either use a bounce function or add a wrapper function.
-   Calling a f(char*) function with f(void*) is non-portable.  */
-typedef void (make_cleanup_ftype) (void *);
+/* Cleanup utilities.  */
 
-extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);
-
-extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
-					  void (*dtor) (void *));
+#include "cleanups.h"
 
 extern struct cleanup *make_cleanup_freeargv (char **);
 
@@ -374,29 +334,10 @@ extern struct cleanup *make_cleanup_valu
 struct so_list;
 extern struct cleanup *make_cleanup_free_so (struct so_list *so);
 
-extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
-
-extern struct cleanup *make_my_cleanup (struct cleanup **,
-					make_cleanup_ftype *, void *);
-
 extern struct cleanup *make_cleanup_htab_delete (htab_t htab);
 
-extern struct cleanup *make_my_cleanup2 (struct cleanup **,
-					 make_cleanup_ftype *, void *,
-					 void (*free_arg) (void *));
-
-extern struct cleanup *save_cleanups (void);
-extern struct cleanup *save_final_cleanups (void);
-extern struct cleanup *save_my_cleanups (struct cleanup **);
-
-extern void restore_cleanups (struct cleanup *);
-extern void restore_final_cleanups (struct cleanup *);
-extern void restore_my_cleanups (struct cleanup **, struct cleanup *);
-
 extern void free_current_contents (void *);
 
-extern void null_cleanup (void *);
-
 extern struct cleanup *make_command_stats_cleanup (int);
 
 extern int myread (int, char *, int);
Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.274
diff -u -p -r1.274 utils.c
--- utils.c	7 Feb 2012 04:48:22 -0000	1.274
+++ utils.c	15 Apr 2012 19:23:28 -0000
@@ -99,8 +99,6 @@ static void vfprintf_maybe_filtered (str
 
 static void fputs_maybe_filtered (const char *, struct ui_file *, int);
 
-static void do_my_cleanups (struct cleanup **, struct cleanup *);
-
 static void prompt_for_continue (void);
 
 static void set_screen_size (void);
@@ -110,12 +108,6 @@ static void set_width (void);
 
 static int debug_timestamp = 0;
 
-/* Chain of cleanup actions established with make_cleanup,
-   to be executed if an error happens.  */
-
-static struct cleanup *cleanup_chain;	/* cleaned up after a failed command */
-static struct cleanup *final_cleanup_chain;	/* cleaned up when gdb exits */
-
 /* Nonzero if we have job control.  */
 
 int job_control;
@@ -172,31 +164,11 @@ show_pagination_enabled (struct ui_file 
 }
 
 \f
+/* Cleanup utilities.
 
-/* 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.  */
-
-struct cleanup *
-make_cleanup (make_cleanup_ftype *function, void *arg)
-{
-  return make_my_cleanup (&cleanup_chain, function, arg);
-}
-
-struct cleanup *
-make_cleanup_dtor (make_cleanup_ftype *function, void *arg,
-		   void (*dtor) (void *))
-{
-  return make_my_cleanup2 (&cleanup_chain,
-			   function, arg, dtor);
-}
-
-struct cleanup *
-make_final_cleanup (make_cleanup_ftype *function, void *arg)
-{
-  return make_my_cleanup (&final_cleanup_chain, function, arg);
-}
+   These are not defined in cleanups.c (nor declared in cleanups.h)
+   because while they use the "cleanup API" they are not part of the
+   "cleanup API".  */
 
 static void
 do_freeargv (void *arg)
@@ -207,7 +179,7 @@ do_freeargv (void *arg)
 struct cleanup *
 make_cleanup_freeargv (char **arg)
 {
-  return make_my_cleanup (&cleanup_chain, do_freeargv, arg);
+  return make_cleanup (do_freeargv, arg);
 }
 
 static void
@@ -219,7 +191,7 @@ do_dyn_string_delete (void *arg)
 struct cleanup *
 make_cleanup_dyn_string_delete (dyn_string_t arg)
 {
-  return make_my_cleanup (&cleanup_chain, do_dyn_string_delete, arg);
+  return make_cleanup (do_dyn_string_delete, arg);
 }
 
 static void
@@ -296,7 +268,7 @@ do_ui_file_delete (void *arg)
 struct cleanup *
 make_cleanup_ui_file_delete (struct ui_file *arg)
 {
-  return make_my_cleanup (&cleanup_chain, do_ui_file_delete, arg);
+  return make_cleanup (do_ui_file_delete, arg);
 }
 
 /* Helper function for make_cleanup_ui_out_redirect_pop.  */
@@ -316,7 +288,7 @@ do_ui_out_redirect_pop (void *arg)
 struct cleanup *
 make_cleanup_ui_out_redirect_pop (struct ui_out *uiout)
 {
-  return make_my_cleanup (&cleanup_chain, do_ui_out_redirect_pop, uiout);
+  return make_cleanup (do_ui_out_redirect_pop, uiout);
 }
 
 static void
@@ -328,7 +300,7 @@ do_free_section_addr_info (void *arg)
 struct cleanup *
 make_cleanup_free_section_addr_info (struct section_addr_info *addrs)
 {
-  return make_my_cleanup (&cleanup_chain, do_free_section_addr_info, addrs);
+  return make_cleanup (do_free_section_addr_info, addrs);
 }
 
 struct restore_integer_closure
@@ -357,8 +329,7 @@ make_cleanup_restore_integer (int *varia
   c->variable = variable;
   c->value = *variable;
 
-  return make_my_cleanup2 (&cleanup_chain, restore_integer, (void *)c,
-			   xfree);
+  return make_cleanup_dtor (restore_integer, (void *) c, xfree);
 }
 
 /* Remember the current value of *VARIABLE and make it restored when
@@ -385,7 +356,7 @@ do_unpush_target (void *arg)
 struct cleanup *
 make_cleanup_unpush_target (struct target_ops *ops)
 {
-  return make_my_cleanup (&cleanup_chain, do_unpush_target, ops);
+  return make_cleanup (do_unpush_target, ops);
 }
 
 /* Helper for make_cleanup_htab_delete compile time checking the types.  */
@@ -448,7 +419,7 @@ do_value_free_to_mark (void *value)
 struct cleanup *
 make_cleanup_value_free_to_mark (struct value *mark)
 {
-  return make_my_cleanup (&cleanup_chain, do_value_free_to_mark, mark);
+  return make_cleanup (do_value_free_to_mark, mark);
 }
 
 /* Helper for make_cleanup_value_free.  */
@@ -464,7 +435,7 @@ do_value_free (void *value)
 struct cleanup *
 make_cleanup_value_free (struct value *value)
 {
-  return make_my_cleanup (&cleanup_chain, do_value_free, value);
+  return make_cleanup (do_value_free, value);
 }
 
 /* Helper for make_cleanup_free_so.  */
@@ -482,133 +453,7 @@ do_free_so (void *arg)
 struct cleanup *
 make_cleanup_free_so (struct so_list *so)
 {
-  return make_my_cleanup (&cleanup_chain, do_free_so, so);
-}
-
-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;
-}
-
-struct cleanup *
-make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function,
-		 void *arg)
-{
-  return make_my_cleanup2 (pmy_chain, function, arg, NULL);
-}
-
-/* Discard cleanups and do the actions they describe
-   until we get back to the point OLD_CHAIN in the cleanup_chain.  */
-
-void
-do_cleanups (struct cleanup *old_chain)
-{
-  do_my_cleanups (&cleanup_chain, old_chain);
-}
-
-void
-do_final_cleanups (struct cleanup *old_chain)
-{
-  do_my_cleanups (&final_cleanup_chain, old_chain);
-}
-
-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);
-    }
-}
-
-/* Discard cleanups, not doing the actions they describe,
-   until we get back to the point OLD_CHAIN in the cleanup_chain.  */
-
-void
-discard_cleanups (struct cleanup *old_chain)
-{
-  discard_my_cleanups (&cleanup_chain, old_chain);
-}
-
-void
-discard_final_cleanups (struct cleanup *old_chain)
-{
-  discard_my_cleanups (&final_cleanup_chain, old_chain);
-}
-
-void
-discard_my_cleanups (struct cleanup **pmy_chain,
-		     struct cleanup *old_chain)
-{
-  struct cleanup *ptr;
-
-  while ((ptr = *pmy_chain) != old_chain)
-    {
-      *pmy_chain = ptr->next;
-      if (ptr->free_arg)
-	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
-    }
-}
-
-/* Set the cleanup_chain to 0, and return the old cleanup chain.  */
-struct cleanup *
-save_cleanups (void)
-{
-  return save_my_cleanups (&cleanup_chain);
-}
-
-struct cleanup *
-save_final_cleanups (void)
-{
-  return save_my_cleanups (&final_cleanup_chain);
-}
-
-struct cleanup *
-save_my_cleanups (struct cleanup **pmy_chain)
-{
-  struct cleanup *old_chain = *pmy_chain;
-
-  *pmy_chain = 0;
-  return old_chain;
-}
-
-/* Restore the cleanup chain from a previously saved chain.  */
-void
-restore_cleanups (struct cleanup *chain)
-{
-  restore_my_cleanups (&cleanup_chain, chain);
-}
-
-void
-restore_final_cleanups (struct cleanup *chain)
-{
-  restore_my_cleanups (&final_cleanup_chain, chain);
-}
-
-void
-restore_my_cleanups (struct cleanup **pmy_chain, struct cleanup *chain)
-{
-  *pmy_chain = chain;
+  return make_cleanup (do_free_so, so);
 }
 
 /* This function is useful for cleanups.
@@ -634,18 +479,6 @@ free_current_contents (void *ptr)
     }
 }
 
-/* Provide a known function that does nothing, to use as a base for
-   a possibly long chain of cleanups.  This is useful where we
-   use the cleanup chain for handling normal cleanups as well as dealing
-   with cleanups that need to be done as a result of a call to error().
-   In such cases, we may not be certain where the first cleanup is, unless
-   we have a do-nothing one to always use as the base.  */
-
-void
-null_cleanup (void *arg)
-{
-}
-
 /* If nonzero, display time usage both at startup and for each command.  */
 
 static int display_time;

Re: [patch] Create cleanups.[ch]

[Sergio Durigan Junior]

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

Re: [patch] Create cleanups.[ch]

[Pedro Alves]

On 04/16/2012 06:00 AM, Sergio Durigan Junior wrote:
> 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!).

Note any such changes should best be done as separate changes.  It's best
to separate code motion from other changes.

This made me notice that make_my_cleanup was unexported, and made static in
this patch, but note how that goes by mostly unnoticed (at least it was
to me on first sight).  I'd have preferred that had been done as an independent
preparatory patch.  I've now checked insight/gdbtk, to see if it was making
using use of it of those symbols, just in case, which would mean
in addition that CLEANUP_FENCEPOST in the other patch would also need to
be exported, and, it's not.

> It should be:

> 
> Copyright (C) 2012 Free Software Foundation, Inc.
> 
> AFAIK, since it's a new file.


But the contents are not new.  By that reasoning, if we renamed
every file in the tree, while changing nothing else, all the copyright
years would end up 2012 only.  I don't think that'd be right.

-- 
Pedro Alves

Re: [patch] Create cleanups.[ch]

[Sergio Durigan Junior]

On Monday, April 16 2012, Pedro Alves wrote:

> On 04/16/2012 06:00 AM, Sergio Durigan Junior wrote:
>> It should be:
>
>> 
>> Copyright (C) 2012 Free Software Foundation, Inc.
>> 
>> AFAIK, since it's a new file.
>
>
> But the contents are not new.  By that reasoning, if we renamed
> every file in the tree, while changing nothing else, all the copyright
> years would end up 2012 only.  I don't think that'd be right.

I always thought that the Copyright referred to the file itself, so if
you create a new file you should "restart" the Copyright notice even if
this file doesn't contain new things.  After all, if you moved things
around, they were already covered by the copyright of the file which
they belonged before.

Anyway, I'm not an expert, so thanks for clarifying.

-- 
Sergio

Re: [patch] Create cleanups.[ch]

[Pedro Alves]

On 04/16/2012 07:50 PM, Sergio Durigan Junior wrote:

> On Monday, April 16 2012, Pedro Alves wrote:
> 
>> On 04/16/2012 06:00 AM, Sergio Durigan Junior wrote:
>>> It should be:
>>
>>>
>>> Copyright (C) 2012 Free Software Foundation, Inc.
>>>
>>> AFAIK, since it's a new file.
>>
>>
>> But the contents are not new.  By that reasoning, if we renamed
>> every file in the tree, while changing nothing else, all the copyright
>> years would end up 2012 only.  I don't think that'd be right.
> 
> I always thought that the Copyright referred to the file itself, so if
> you create a new file you should "restart" the Copyright notice even if
> this file doesn't contain new things.  After all, if you moved things
> around, they were already covered by the copyright of the file which
> they belonged before.


But then if you follow that patch one extra step, you can rename
the files back to their original names, and what you end up with
is exactly with you started with, except you've lost all your copyright
years.  If that's okay, then I've just proven that a copyright year list
is insignificant and unnecessary, and we can just go do a wholesale pass
on the whole codebase replacing all the multiple years by a single
"2012" year.

> Anyway, I'm not an expert, so thanks for clarifying.


I'm not an expert either.  Better ask the FSF indeed.  But please do
raise my point above with them.

-- 
Pedro Alves

Re: [patch] Create cleanups.[ch]

[Sergio Durigan Junior]

On Wednesday, April 18 2012, Pedro Alves wrote:

> On 04/16/2012 07:50 PM, Sergio Durigan Junior wrote:
>
>> I always thought that the Copyright referred to the file itself, so if
>> you create a new file you should "restart" the Copyright notice even if
>> this file doesn't contain new things.  After all, if you moved things
>> around, they were already covered by the copyright of the file which
>> they belonged before.
>
>
> But then if you follow that patch one extra step, you can rename
> the files back to their original names, and what you end up with
> is exactly with you started with, except you've lost all your copyright
> years.  If that's okay, then I've just proven that a copyright year list
> is insignificant and unnecessary, and we can just go do a wholesale pass
> on the whole codebase replacing all the multiple years by a single
> "2012" year.

Turns out you are right.  Here is the reply from FSF:

----

From: "Donald R Robertson III via RT" <copyright-clerk@fsf.org>
Subject: [gnu.org #745974] Question about Copyright notice on files 
To: sergiodj@redhat.com
Date: Thu, 19 Apr 2012 13:01:52 -0400
Reply-To: copyright-clerk@fsf.org
X-Sent: 22 minutes, 40 seconds ago

Hello,

Thanks for checking in on this.

> [sergiodj@redhat.com - Wed Apr 18 01:59:09 2012]:
> 
> Hello,
> 
> On behalf of the GDB Project, I am sending you a question that arose
> while discussing some patch in the gdb-patches mailing list.  The
> question is about the Copyright notice on files of the project.  If you
> are interested in the discussion, please have a look at this specific
> thread:
> 
> > http://sourceware.org/ml/gdb-patches/2012-04/msg00395.html> 
> To make it "short": we would like to know if, when we move an already
> existing piece of code from one file (say `old-file.c', for example) to
> another, newly created file (say `new-file.c', which was created during
> this code movement), what should be the year specified in the Copyright
> notice above the file?  If `old-file.c' has a copyright like this:
> 
> 
> > Copyright (C) 1986, 1988-2005, 2007-2012 Free Software Foundation, Inc.
> 
> Should the `new-file.c' contain these same years on its own Copyright
> notice, or should it contain only the year when it was created, e.g.,
> supposing `new-file.c' was created today, should it display:
> 
> > Copyright (C) 2012 Free Software Foundation, Inc.
> 
> as the notice?
> 
> Please correct me if I'm wrong, but if you prefer, maybe you can
> interpret this question as: "Is the Copyright notice related to the
> contents of the file, or to the file itself?"
> 
> Thank you very much,

The short answer is that the notice relates to what is in the file.

This is the relevant section in the maintainers guide 
(http://www.gnu.org/prep/maintain/maintain.html#Copyright-Notices):

"To update the list of year numbers, add each year in which you have made nontrivial changes to 
the package. (Here we assume you’re using a publicly accessible revision control server, so that 
every revision installed is also immediately and automatically published.) When you add the new 
year, it is not required to keep track of which files have seen significant changes in the new 
year and which have not. It is recommended and simpler to add the new year to all files in the 
package, and be done with it for the rest of the year.

Don’t delete old year numbers, though; they are significant since they indicate when older 
versions might theoretically go into the public domain, if the movie companies don’t continue 
buying laws to further extend copyright. If you copy a file into the package from some other 
program, keep the copyright years that come with the file."

As you can see, it turns on when changes are made, not when the file is created. Since the code 
itself was created in previous years, we need to maintain those years in the copyright notice. 
So in your example, the notice should read like this:

Copyright (C) 1986, 1988-2005, 2007-2012 Free Software Foundation, Inc.

As the manual notes, you can just add the most recent year to all non-trivial files, without 
divining whether each individual file has been updated. I hope this answers your question but 
let me know if you need any more information.

> 
> 
> 
-- 
Sincerely,

Donald R. Robertson, III, J.D.
Assignment Administrator
Free Software Foundation
51 Franklin Street, Fifth Floor
Boston, MA 02110
Phone +1-617-542-5942 
Fax +1-617-542-2652

---

-- 
Sergio

Re: [patch] Create cleanups.[ch]

[Pedro Alves]

On 04/19/2012 06:32 PM, Sergio Durigan Junior wrote:

> ... Here is the reply from FSF:


Thanks for getting this clarified.

-- 
Pedro Alves

Re: [patch] Create cleanups.[ch]

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This made me notice that make_my_cleanup was unexported, and made
Pedro> static in this patch, but note how that goes by mostly unnoticed
Pedro> (at least it was to me on first sight).

I have long had the impression, based solely on reading the cleanup
code, that cleanups were intended to be used in other ways and then
never were.  E.g., unchaining cleanups and attaching them to some other
object (say, replacing the existing objfile data destructor methods)
seems doable, but AFAIK is not ever done.

If anything is unused at this point, I'd say we can just delete it.

Tom

Re: [patch] Create cleanups.[ch]

[Pedro Alves]

On 04/16/2012 09:06 PM, Tom Tromey wrote:

>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> This made me notice that make_my_cleanup was unexported, and made
> Pedro> static in this patch, but note how that goes by mostly unnoticed
> Pedro> (at least it was to me on first sight).
> 
> I have long had the impression, based solely on reading the cleanup
> code, that cleanups were intended to be used in other ways and then
> never were.  E.g., unchaining cleanups and attaching them to some other
> object (say, replacing the existing objfile data destructor methods)
> seems doable, but AFAIK is not ever done.


Makes sense.  It never occurred to me.

> If anything is unused at this point, I'd say we can just delete it.


Sorry if it sounded like I didn't agree with that; I definitely do.
I only meant that it'd have preferred such change as a separate change.
It's okay with me to not bother now.

-- 
Pedro Alves

Re: [patch] Create cleanups.[ch]

[Tom Tromey]

Pedro> I only meant that it'd have preferred such change as a separate change.

Yeah, me too.

Tom

Re: [patch] Create cleanups.[ch]

[Doug Evans]

On Mon, Apr 16, 2012 at 1:42 PM, Tom Tromey <tromey@redhat.com> wrote:
> Pedro> I only meant that it'd have preferred such change as a separate change.
>
> Yeah, me too.
>
> Tom

Making it a separate change meant splitting it into three patches instead of one
(unless I either artificially made cleanup_chain non-static for one
patch, blech, or modified the remaining uses in utils.c to not
reference it, but then it would still be more than just a simple
move).  That is why I went that route this time.

But whatever, three patches checked in (after a whole pile of extra effort).

One to go.

Re: [patch] Create cleanups.[ch]

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> I also never know what's the best/recommended practice: to put the
Sergio> comments above the function's declaration (in this case, in the
Sergio> cleanups.h file), or to put comments above the function definition (as
Sergio> you did).  Maybe someone more experienced can clarify.

There's no consensus.

Tom

Re: [patch] Create cleanups.[ch]

[Doug Evans]

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.

Re: [patch] Create cleanups.[ch]

[Joel Brobecker]

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

I wasn't aware of any preference either. Perhaps an idiomatic/linguistic
thing? Like maybe "ditto" sounds weird in some regions. It's the type of
detail that really doesn't deserve much attention, if you ask me.

Anyways, the real reason for me replying to this email:

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

Have we considered the option of asking advice from the FSF about this?
Then we'd have a definitive answer which we can record as well (put that
in our CS guide, perhaps in gdbint.texi or in the Wiki). I've asked
a couple of questions already this year, so I feel a little hesitant
to ask myself (it's silly I know, just can't help it).

-- 
Joel

Re: [patch] Create cleanups.[ch]

[Sergio Durigan Junior]

On Tuesday, April 17 2012, Joel Brobecker wrote:

>> > 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."
>
> Have we considered the option of asking advice from the FSF about this?
> Then we'd have a definitive answer which we can record as well (put that
> in our CS guide, perhaps in gdbint.texi or in the Wiki). I've asked
> a couple of questions already this year, so I feel a little hesitant
> to ask myself (it's silly I know, just can't help it).

I can gladly ask :-).  What's the e-mail/procedure?  Should I Cc them in
this e-mail?

Thanks,

-- 
Sergio

Re: [patch] Create cleanups.[ch]

[Joel Brobecker]

> I can gladly ask :-).  What's the e-mail/procedure?  Should I Cc them in
> this e-mail?

For the record, and to avoid unnecessary duplication of effort,
I told Sergio to send an email to copyright-clerk.

-- 
Joel

Re: [patch] Create cleanups.[ch]

[Pedro Alves]

On 04/15/2012 08:42 PM, Doug Evans wrote:

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


+1.

I was even meaning to do this myself.  Thanks.

-- 
Pedro Alves

Re: [patch] Create cleanups.[ch]

[Yao Qi]

On 04/16/2012 03:42 AM, Doug Evans wrote:
> 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.

When hacking GDBserver, I am wondering if we plan to use cleanup in
GDBserver?  It is quite convenient if we use cleanup in GDBserver.

-- 
Yao (齐尧)

Re: [patch] Create cleanups.[ch]

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> On 04/16/2012 03:42 AM, Doug Evans wrote:
>> 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.

Yao> When hacking GDBserver, I am wondering if we plan to use cleanup in
Yao> GDBserver?  It is quite convenient if we use cleanup in GDBserver.

IIRC Pedro said as much somewhere in the C++ thread.

Tom