Re: RFA: Patch: implement missing macro functions

[Pedro Alves <pedro at codesourcery dot 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