Re: RFA: Patch: implement missing macro functions

[Pedro Alves <pedro@codesourcery.com>]

Hi Tom,

You'll need someone else to comment on the big picture -- I'll comment
on a few strokes, below,

>  static void
>  macro_define_command (char *exp, int from_tty)
>  {
> -  error (_("Command not implemented yet."));
> +  struct macro_definition *new_macro = NULL;
> +  char *name = NULL;
> +  struct cleanup *cleanup_chain = make_cleanup (free_macro_definition_ptr,
> +                                               &new_macro);


> +  cleanup_chain = make_my_cleanup (&cleanup_chain, free_current_contents,
> +                                  &name);

That is a big nop to write + with 2 leaks attached.  The new cleanup is
being discarded and leaked, so NAME isn't being free'd at all.

Don't call make_my_cleanup directly.  Instead, call make_cleanup and discard
its result:

You want to hold the cleanup chain pointer representing the old
cleanups on function entry, and that is returned on the first make_cleanup
call.

struct cleanup *cleanup_chain = make_cleanup (free_macro_definition_ptr,
                                               &new_macro);
make_cleanup (free_current_contents, &name);

(pedantically, if you don't need the NAME variable after cleaning up or
an exception is thrown (local vars usually apply), an xfree cleanup
is also fine, like in `make_cleanup (xfree, name)')

> +static void
> +print_one_macro (const char *name, const struct macro_definition *macro)
> +{
> +  fprintf_filtered (gdb_stdout, "macro define %s", name);
> +  if (macro->kind == macro_function_like)
> +    {
> +      int i;
> +      fprintf_filtered (gdb_stdout, "(");
> +      for (i = 0; i < macro->argc; ++i)
> +       fprintf_filtered (gdb_stdout, "%s%s", (i > 0) ? ", " : "",
> +                         macro->argv[i]);
> +      fprintf_filtered (gdb_stdout, ")");
> +    }
> +  /* Note that we don't need a leading space here -- "macro define"
> +     provided it.  */
> +  fprintf_filtered (gdb_stdout, "%s\n", macro->replacement);
> +  gdb_flush (gdb_stdout);
>  }

Did you really need a gdb_flush here?

>  
> +/* Helper function for macro_for_each.  */
> +static int
> +foreach_macro (splay_tree_node node, void *fnp)
> +{
> +  macro_callback_fn fn = (macro_callback_fn) fnp;

(Hmmm, can we assume casting void* <-> func pointers is safe on all
supported hosts/compilers?  Posix and Windows do require it to
be safe.  I actually have no idea how much GDB common code relies
on it today.)

> +  struct macro_key *key = (struct macro_key *) node->key;
> +  struct macro_definition *def = (struct macro_definition *) node->value;
> +  (*fn) (key->name, def);
> +  return 0;
> +}
> +
> +void
> +macro_for_each (struct macro_table *table, macro_callback_fn fn)
> +{
> +  splay_tree_foreach (table->definitions, foreach_macro, fn);
> +}


>  
>  #endif /* MACROTAB_H */
> Index: testsuite/gdb.base/macscp.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/macscp.exp,v
> retrieving revision 1.7
> diff -u -r1.7 macscp.exp
> --- testsuite/gdb.base/macscp.exp       3 May 2008 22:30:51 -0000       1.7
> +++ testsuite/gdb.base/macscp.exp       21 May 2008 00:25:09 -0000
> @@ -428,3 +428,23 @@
>  gdb_test "print M" \
>      "No symbol \"M\" in current context\." \
>      "print expression with macro after undef."
> +
> +gdb_test "macro define M 5" \
> +  "" \
> +  "basic macro define"
> +
> +gdb_test "print M" \
> +  " = 5" \
> +  "expansion of defined macro"
> +
> +gdb_test "macro list" \
> +  "macro define M 5" \
> +  "basic macro list"
> +
> +gdb_test "macro undef M" \
> +  "" \
> +  "basic macro undef"
> +
> +gdb_test "print M" \
> +    "No symbol \"M\" in current context\." \
> +    "print expression with macro after user undef."

Would it make sense to have a test with macro arguments, and
a test where a user macro overrides a source macro?

-- 
Pedro Alves