Re: [PATCH 2/9] add registry.h, change existing users

[Pedro Alves <palves at redhat dot com>]

On 07/30/2012 04:20 PM, Tom Tromey wrote:
> We use the "registry" pattern in a few places in gdb -- objfiles,
> program spaces, and inferiors.  Mostly this is done by a lot of
> copy-and-paste, plus renamings.
> 
> I wanted to add a registry to BFDs, so I took the opportunity to
> replace the copy-and-paste with some macros, akin to vec.h.
> 
> This patch introduces registry.h and updates all the existing uses to
> use it.

I like it.

One thing I notice is that this leave everything templatized,
while it is possible to factor most things out, leaving only the templating
as a shim (like in C++ you would push the common code to a base class that
handles void*, and then derive a template shim on top that just converts
pointers).  Something like this.  This would impact your following
patches, so if you prefer and agree it's a good idea, I'll get back
to it only after your patches are in.

gdb/Makefile.in |    2 -
 gdb/registry.c  |  116 ++++++++++++++++++++++++++++++++++
 gdb/registry.h  |  190 +++++++++++++++++++++++++++++++++----------------------
 3 files changed, 232 insertions(+), 76 deletions(-)
 create mode 100644 gdb/registry.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 06f93ce..0920e17 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -919,7 +919,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	inferior.o osdata.o gdb_usleep.o record.o gcore.o \
 	gdb_vecs.o jit.o progspace.o skip.o probe.o \
 	common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
-	format.o
+	format.o registry.o

 TSOBS = inflow.o

diff --git a/gdb/registry.c b/gdb/registry.c
new file mode 100644
index 0000000..c01929b
--- /dev/null
+++ b/gdb/registry.c
@@ -0,0 +1,116 @@
+/* Support functions for general registry objects.
+
+   Copyright (C) 2011, 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"
+#include "registry.h"
+#include "gdb_assert.h"
+#include "gdb_string.h"
+
+const struct registry_data *
+register_data_with_cleanup (struct registry_data_registry *registry,
+			    registry_data_callback save,
+			    registry_data_callback free)
+{
+  struct registry_data_registration **curr;
+
+  /* Append new registration.  */
+  for (curr = &registry->registrations;
+       *curr != NULL;
+       curr = &(*curr)->next)
+    ;
+
+  *curr = XMALLOC (struct registry_data_registration);
+  (*curr)->next = NULL;
+  (*curr)->data = XMALLOC (struct registry_data);
+  (*curr)->data->index = registry->num_registrations++;
+  (*curr)->data->save = save;
+  (*curr)->data->free = free;
+
+  return (*curr)->data;
+}
+
+void
+registry_alloc_data (struct registry_data_registry *registry,
+		       struct registry_fields *fields)
+{
+  gdb_assert (fields->data == NULL);
+  fields->num_data = registry->num_registrations;
+  fields->data = XCALLOC (fields->num_data, void *);
+}
+
+void
+registry_clear_data (struct registry_data_registry *data_registry,
+		     registry_callback_adaptor adaptor,
+		     struct registry_container *container,
+		     struct registry_fields *fields)
+{
+  struct registry_data_registration *registration;
+  int i;
+
+  gdb_assert (fields->data != NULL);
+
+  /* Process all the save handlers.  */
+
+  for (registration = data_registry->registrations, i = 0;
+       i < fields->num_data;
+       registration = registration->next, i++)
+    if (fields->data[i] != NULL && registration->data->save != NULL)
+      adaptor (registration->data->save, container, fields->data[i]);
+
+  /* Now process all the free handlers.  */
+
+  for (registration = data_registry->registrations, i = 0;
+       i < fields->num_data;
+       registration = registration->next, i++)
+    if (fields->data[i] != NULL && registration->data->free != NULL)
+      adaptor (registration->data->free, container, fields->data[i]);
+
+  memset (fields->data, 0, fields->num_data * sizeof (void *));
+}
+
+void
+registry_container_free_data (struct registry_data_registry *data_registry,
+			      registry_callback_adaptor adaptor,
+			      struct registry_container *container,
+			      struct registry_fields *fields)
+{
+  void ***rdata = &fields->data;
+  gdb_assert (*rdata != NULL);
+  registry_clear_data (data_registry, adaptor, container, fields);
+  xfree (*rdata);
+  *rdata = NULL;
+}
+
+void
+registry_set_data (struct registry_fields *fields,
+		   const struct registry_data *data,
+		   void *value)
+{
+  gdb_assert (data->index < fields->num_data);
+  fields->data[data->index] = value;
+}
+
+void *
+registry_data (struct registry_fields *fields,
+	       const struct registry_data *data)
+{
+  gdb_assert (data->index < fields->num_data);
+  return fields->data[data->index];
+}
diff --git a/gdb/registry.h b/gdb/registry.h
index d696781..555f26f 100644
--- a/gdb/registry.h
+++ b/gdb/registry.h
@@ -57,52 +57,96 @@
 /* This macro is used in a container struct definition to define the
    fields used by the registry code.  */

-#define REGISTRY_FIELDS				\
-  void **data;					\
-  unsigned num_data
+#define REGISTRY_FIELDS \
+  struct registry_fields registry_fields
+
+/* The fields used by the registry code put in a container struct
+   definition.  */
+struct registry_fields
+{
+  void **data;
+  unsigned num_data;
+};
+
+/* Opaque type representing a container type with a registry.  This
+   type is never defined.  This is used to factor out common
+   functionality of all struct tag names into common code.  IOW,
+   "struct tag name" pointers are cast to and from "struct
+   registry_container" pointers when calling the common registry
+   "backend" functions.  */
+struct registry_container;
+
+/* Registry callbacks have this this.  */
+typedef void (*registry_data_callback) (struct registry_container *, void *);
+
+struct registry_data
+{
+  unsigned index;
+  registry_data_callback save;
+  registry_data_callback free;
+};
+
+struct registry_data_registration
+{
+  struct registry_data *data;
+  struct registry_data_registration *next;
+};
+
+struct registry_data_registry
+{
+  struct registry_data_registration *registrations;
+  unsigned num_registrations;
+};
+
+/* Registry backend functions.  Client code uses the frontend
+   functions defined by DEFINE_REGISTRY below.  */
+
+const struct registry_data * register_data_with_cleanup
+  (struct registry_data_registry *registry,
+   registry_data_callback save,
+   registry_data_callback free);
+
+void registry_alloc_data (struct registry_data_registry *registry,
+			  struct registry_fields *registry_fields);
+
+/* Cast FUNC and CONTAINER to the real types, and call FUNC, also
+   passing DATA.  */
+typedef void (*registry_callback_adaptor) (registry_data_callback func,
+					   struct registry_container *container,
+					   void *data);
+
+void registry_clear_data (struct registry_data_registry *data_registry,
+			  registry_callback_adaptor adaptor,
+			  struct registry_container *container,
+			  struct registry_fields *fields);
+
+void registry_container_free_data (struct registry_data_registry *data_registry,
+				   registry_callback_adaptor adaptor,
+				   struct registry_container *container,
+				   struct registry_fields *fields);
+
+void registry_set_data (struct registry_fields *fields,
+			const struct registry_data *data,
+			void *value);
+
+void *registry_data (struct registry_fields *fields,
+		     const struct registry_data *data);

 /* Define a new registry implementation.  */

 #define DEFINE_REGISTRY(TAG)						\
-struct TAG ## _data							\
-{									\
-  unsigned index;							\
-  void (*save) (struct TAG *, void *);					\
-  void (*free) (struct TAG *, void *);					\
-};									\
-									\
-struct TAG ## _data_registration					\
-{									\
-  struct TAG ## _data *data;						\
-  struct TAG ## _data_registration *next;				\
-};									\
-									\
-struct TAG ## _data_registry						\
-{									\
-  struct TAG ## _data_registration *registrations;			\
-  unsigned num_registrations;						\
-};									\
-									\
-struct TAG ## _data_registry TAG ## _data_registry = { NULL, 0 };	\
+struct registry_data_registry TAG ## _data_registry = { NULL, 0 };	\
 									\
 const struct TAG ## _data *						\
 register_ ## TAG ## _data_with_cleanup (void (*save) (struct TAG *, void *), \
-				    void (*free) (struct TAG *, void *)) \
+					void (*free) (struct TAG *, void *)) \
 {									\
-  struct TAG ## _data_registration **curr;				\
-									\
-  /* Append new registration.  */					\
-  for (curr = &TAG ## _data_registry.registrations;			\
-       *curr != NULL; curr = &(*curr)->next);				\
-									\
-  *curr = XMALLOC (struct TAG ## _data_registration);			\
-  (*curr)->next = NULL;							\
-  (*curr)->data = XMALLOC (struct TAG ## _data);			\
-  (*curr)->data->index = TAG ## _data_registry.num_registrations++;	\
-  (*curr)->data->save = save;						\
-  (*curr)->data->free = free;						\
+  struct registry_data_registration **curr;				\
 									\
-  return (*curr)->data;							\
+  return (struct TAG ## _data *)					\
+    register_data_with_cleanup (&TAG ## _data_registry,			\
+				(registry_data_callback) save,		\
+				(registry_data_callback) free);		\
 }									\
 									\
 const struct TAG ## _data *						\
@@ -114,73 +158,69 @@ register_ ## TAG ## _data (void)					\
 static void								\
 TAG ## _alloc_data (struct TAG *container)				\
 {									\
-  gdb_assert (container->data == NULL);					\
-  container->num_data = TAG ## _data_registry.num_registrations;	\
-  container->data = XCALLOC (container->num_data, void *);		\
+  registry_alloc_data (&TAG ## _data_registry, &container->registry_fields); \
 }									\
 									\
-void									\
-clear_ ## TAG ## _data (struct TAG *container)				\
+static void								\
+TAG ## registry_callback_adaptor (registry_data_callback func,		\
+				  struct registry_container *container, \
+				  void *data)				\
 {									\
-  struct TAG ## _data_registration *registration;			\
-  int i;								\
-									\
-  gdb_assert (container->data != NULL);					\
-									\
-  /* Process all the save handlers.  */					\
-									\
-  for (registration = TAG ## _data_registry.registrations, i = 0;	\
-       i < container->num_data;						\
-       registration = registration->next, i++)				\
-    if (container->data[i] != NULL && registration->data->save != NULL)	\
-      registration->data->save (container, container->data[i]);		\
-									\
-  /* Now process all the free handlers.  */				\
+  registry_ ## TAG ## _callback tagged_func				\
+    = (registry_ ## TAG ## _callback) func;				\
+  struct TAG *tagged_container = (struct TAG *) container;		\
 									\
-  for (registration = TAG ## _data_registry.registrations, i = 0;	\
-       i < container->num_data;						\
-       registration = registration->next, i++)				\
-    if (container->data[i] != NULL && registration->data->free != NULL)	\
-      registration->data->free (container, container->data[i]);		\
+  tagged_func (tagged_container, data);					\
+}									\
 									\
-  memset (container->data, 0, container->num_data * sizeof (void *));	\
+void									\
+clear_ ## TAG ## _data (struct TAG *container)				\
+{									\
+ registry_clear_data (&TAG ## _data_registry,				\
+		      TAG ## registry_callback_adaptor,			\
+		      (struct registry_container *) container,		\
+		      &container->registry_fields);			\
 }									\
 									\
 static void								\
 TAG ## _free_data (struct TAG *container)				\
 {									\
-  void ***rdata = &container->data;					\
-  gdb_assert (*rdata != NULL);						\
-  clear_ ## TAG ## _data (container);					\
-  xfree (*rdata);							\
-  *rdata = NULL;							\
+ registry_container_free_data (&TAG ## _data_registry,			\
+			       TAG ## registry_callback_adaptor,	\
+			       (struct registry_container *) container, \
+			       &container->registry_fields);		\
 }									\
 									\
 void									\
-set_ ## TAG ## _data (struct TAG *container, const struct TAG ## _data *data, \
-		  void *value)						\
+set_ ## TAG ## _data (struct TAG *container,				\
+		      const struct TAG ## _data *data,			\
+		      void *value)					\
 {									\
-  gdb_assert (data->index < container->num_data);			\
-  container->data[data->index] = value;					\
+  registry_set_data (&container->registry_fields,			\
+		     (struct registry_data *) data,			\
+		     value);						\
 }									\
 									\
 void *									\
 TAG ## _data (struct TAG *container, const struct TAG ## _data *data)	\
 {									\
-  gdb_assert (data->index < container->num_data);			\
-  return container->data[data->index];					\
+  return registry_data (&container->registry_fields,			\
+			(struct registry_data *) data);			\
 }


 /* External declarations for the registry functions.  */

 #define DECLARE_REGISTRY(TAG)						\
+struct TAG ## _data;							\
+typedef void (*registry_ ## TAG ## _callback) (struct TAG *, void *);	\
 extern const struct TAG ## _data *register_ ## TAG ## _data (void);	\
 extern const struct TAG ## _data *register_ ## TAG ## _data_with_cleanup \
- (void (*save) (struct TAG *, void *), void (*free) (struct TAG *, void *)); \
+ (registry_ ## TAG ## _callback save, registry_ ## TAG ## _callback free); \
 extern void clear_ ## TAG ## _data (struct TAG *);		\
 extern void set_ ## TAG ## _data (struct TAG *,			\
-			       const struct TAG ## _data *data, void *value); \
+				  const struct TAG ## _data *data, \
+				  void *value);			\
 extern void *TAG ## _data (struct TAG *,			\
 			   const struct TAG ## _data *data);