Patchwork [RFAv3,2/2] Factorize macro definition code in macrotab.c

login
register
mail settings
Submitter Philippe Waroquiers
Date Jan. 26, 2019, 10:31 p.m.
Message ID <20190126223117.6718-3-philippe.waroquiers@skynet.be>
Download mbox | patch
Permalink /patch/31219/
State New
Headers show

Comments

Philippe Waroquiers - Jan. 26, 2019, 10:31 p.m.
When first fixing splay tree key leaks in macrotab.c, some duplicated code
logic was factorized.
The key leaks will be fixed in libiberty, but the code factorization
is better kept in any case.

gdb/ChangeLog
2019-01-26  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* macrotab.c (macro_define_internal): New function that
	factorizes macro_define_object_internal and macro_define_function
	code.
	(macro_define_object_internal): Use macro_define_internal.
	(macro_define_function): Likewise.
---
 gdb/macrotab.c | 59 +++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)
Simon Marchi - Feb. 6, 2019, 4:12 a.m.
On 2019-01-26 17:31, Philippe Waroquiers wrote:
> When first fixing splay tree key leaks in macrotab.c, some duplicated 
> code
> logic was factorized.
> The key leaks will be fixed in libiberty, but the code factorization
> is better kept in any case.

LGTM, with just a nit:

> +/* A helper function to define a new object-like macro.  */
> +
> +static void
> +macro_define_object_internal (struct macro_source_file *source, int 
> line,
> +			      const char *name, const char *replacement,
> +			      enum macro_special_kind special_kind)
> +{
> +  macro_define_internal (source, line,
> +			 name, macro_object_like,
> +			 special_kind, 0,
> +			 replacement);
> +}

Since it is a pointer parameter, could you change the "0" for "NULL" or 
"nullptr"?

Thanks,

Simon
Philippe Waroquiers - Feb. 6, 2019, 8:08 p.m.
On Tue, 2019-02-05 at 23:12 -0500, Simon Marchi wrote:
> On 2019-01-26 17:31, Philippe Waroquiers wrote:
> > When first fixing splay tree key leaks in macrotab.c, some duplicated 
> > code
> > logic was factorized.
> > The key leaks will be fixed in libiberty, but the code factorization
> > is better kept in any case.
> 
> LGTM, with just a nit:
> 
> > +/* A helper function to define a new object-like macro.  */
> > +
> > +static void
> > +macro_define_object_internal (struct macro_source_file *source, int 
> > line,
> > +			      const char *name, const char *replacement,
> > +			      enum macro_special_kind special_kind)
> > +{
> > +  macro_define_internal (source, line,
> > +			 name, macro_object_like,
> > +			 special_kind, 0,
> > +			 replacement);
> > +}
> 
> Since it is a pointer parameter, could you change the "0" for "NULL" or 
> "nullptr"?
Thanks for the review, I pushed the series after doing the fixes
in this and in the other mail.

Philippe

Patch

diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7fcab4691b..f7178bdc25 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -743,21 +743,26 @@  check_for_redefinition (struct macro_source_file *source, int line,
     return 0;
 }
 
-/* A helper function to define a new object-like macro.  */
+/* A helper function to define a new object-like or function-like macro
+   according to KIND.  When KIND is macro_object_like,
+   the macro_special_kind must be provided as ARGC, and ARGV must be NULL.
+   When KIND is macro_function_like, ARGC and ARGV are giving the function
+   arguments.  */
 
 static void
-macro_define_object_internal (struct macro_source_file *source, int line,
-			      const char *name, const char *replacement,
-			      enum macro_special_kind kind)
+macro_define_internal (struct macro_source_file *source, int line,
+                       const char *name, enum macro_kind kind,
+		       int argc, const char **argv,
+                       const char *replacement)
 {
   struct macro_table *t = source->table;
   struct macro_key *k = NULL;
   struct macro_definition *d;
 
   if (! t->redef_ok)
-    k = check_for_redefinition (source, line, 
-				name, macro_object_like,
-				0, 0,
+    k = check_for_redefinition (source, line,
+				name, kind,
+				argc, argv,
 				replacement);
 
   /* If we're redefining a symbol, and the existing key would be
@@ -774,10 +779,23 @@  macro_define_object_internal (struct macro_source_file *source, int line,
     return;
 
   k = new_macro_key (t, name, source, line);
-  d = new_macro_definition (t, macro_object_like, kind, 0, replacement);
+  d = new_macro_definition (t, kind, argc, argv, replacement);
   splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
 }
 
+/* A helper function to define a new object-like macro.  */
+
+static void
+macro_define_object_internal (struct macro_source_file *source, int line,
+			      const char *name, const char *replacement,
+			      enum macro_special_kind special_kind)
+{
+  macro_define_internal (source, line,
+			 name, macro_object_like,
+			 special_kind, 0,
+			 replacement);
+}
+
 void
 macro_define_object (struct macro_source_file *source, int line,
 		     const char *name, const char *replacement)
@@ -802,29 +820,12 @@  macro_define_function (struct macro_source_file *source, int line,
                        const char *name, int argc, const char **argv,
                        const char *replacement)
 {
-  struct macro_table *t = source->table;
-  struct macro_key *k = NULL;
-  struct macro_definition *d;
-
-  if (! t->redef_ok)
-    k = check_for_redefinition (source, line,
-				name, macro_function_like,
-				argc, argv,
-				replacement);
-
-  /* See comments about duplicate keys in macro_define_object.  */
-  if (k && ! key_compare (k, name, source, line))
-    return;
-
-  /* We should also check here that all the argument names in ARGV are
-     distinct.  */
-
-  k = new_macro_key (t, name, source, line);
-  d = new_macro_definition (t, macro_function_like, argc, argv, replacement);
-  splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+  macro_define_internal (source, line,
+			 name, macro_function_like,
+			 argc, argv,
+			 replacement);
 }
 
-
 void
 macro_undef (struct macro_source_file *source, int line,
              const char *name)