Re: RFA: implement all missing macro expansion features

[Joel Brobecker <brobecker at adacore dot com>]

:REVIEWMAIL:

> 2008-08-22  Tom Tromey  <tromey@redhat.com>
> 
> 	* macrocmd.c (extract_identifier): Add is_parameter argument.
> 	(macro_define_command): Update.
> 	(macro_undef_command): Likewise.
> 	* macroexp.c (stringify): New function.
> 	(find_parameter): Likewise.
> 	(gather_arguments): Add nargs argument.  Handle varargs.
> 	(substitute_args): Add is_varargs and va_arg_name arguments.
> 	Handle varargs, splicing, stringification.  Use find_parameter.
> 	(expand): Handle varargs.

Would anyone who is familiar with C macros like to take a look at
this one too? I looked at it, and it looks fine to me. In fact,
I must compliment Tom on his very nice style with lots of nice comments
explaining what he does. Very nice!

Tom, give it a few more days to see if we have any reaction, and
then go ahead and commit (let's not count the weekend, though).

> 2008-08-21  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/macscp.exp: Add tests for stringification, splicing,
> 	and varargs.

This is OK too.

> 
> diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
> index 8213c0d..9a4fdee 100644
> --- a/gdb/macrocmd.c
> +++ b/gdb/macrocmd.c
> @@ -197,18 +197,36 @@ skip_ws (char **expp)
>      ++*expp;
>  }
>  
> +/* Try to find the bounds of an identifier.  If an identifier is
> +   found, returns a newly allocated string; otherwise returns NULL.
> +   EXPP is a pointer to an input string; it is updated to point to the
> +   text following the identifier.  If IS_PARAMETER is true, this
> +   function will also allow "..." forms as used in varargs macro
> +   parameters.  */
>  static char *
> -extract_identifier (char **expp)
> +extract_identifier (char **expp, int is_parameter)

Thanks for adding the comment describing the function. Can you add an
extra empty line before the function declaration?

> +/* A helper function for substitute_args.  If the token TOK is the
> +   name of a parameter, return the parameter's index.  ARGV is a
> +   vector of all the arguments; ARGC is the number of arguments.  If
> +   TOK is not an argument, return -1.  */
> +static int
> +find_parameter (const struct macro_buffer *tok,
> +		int is_varargs, const struct macro_buffer *va_arg_name,
> +		int argc, const char * const *argv)

Same here. And perhaps if you wouldn't mind describing the IS_VARARGS
and VA_ARG_NAME parameters...

> +      /* Is this token the splicing operator?  */
> +      else if (tok.len == 2
> +	       && tok.text[0] == '#'
> +	       && tok.text[1] == '#')
> +	{
> +	  /* Just ignore a stray token splicing operator.  Really this
> +	     is an error, but there's no reason to penalize the
> +	     user.  */
> +	}

I don't see how this would be penalizing the user.  when could this
error actually happen? I assume that the compiler would reject an
incorrect expression, so it would only happen when defining the
macro in GDB? If it were the case, I would rather error out and
tell the user to fix its expression.  Or perhaps can this happen
after expansion?


-- 
Joel