Re: Warning from cpp on macro argument stringification is missing

[Zack Weinberg <zack at wolery dot cumb dot org>]

On Thu, Sep 07, 2000 at 07:14:14PM -0400, Kaveh R. Ghazi wrote:

...
> Here's the patch so far.  It isn't quite ready for prime time, I have
> the debugging prints still in there and there's no ChangeLog and I
> haven't bootstrapped it yet.  Just looking for some quick comments.

In general this looks like the right idea.  See comments below.

Don't forget to write a test case!

> --- orig/egcs-CVS20000906/gcc/cppmacro.c	Fri Aug 18 09:48:44 2000
> +++ egcs-CVS20000906/gcc/cppmacro.c	Thu Sep  7 18:59:29 2000
> @@ -52,6 +52,9 @@ static const cpp_toklist * save_expansio
>  static unsigned int find_param PARAMS ((const cpp_token *,
>   					const cpp_token *));
>  static cpp_toklist * alloc_macro PARAMS ((cpp_reader *, struct macro_info *));
> +static void _cpp_cktrad_stringification PARAMS ((cpp_reader *,
> +						 const struct macro_info *,
> +						 const cpp_string *));

Style nit: static functions shouldn't have a _cpp_ prefix, and
"cktrad" is a bit obscure.  Call it check_trad_stringification or
something like that.

...

> +/* Warn if a token in `string' matches one of the function macro
> +   arguments in `info'.  This function assumes that the macro is a
> +   function macro and not an object macro.  */
> +static void
> +_cpp_cktrad_stringification (pfile, info, string)
> +     cpp_reader *pfile;
> +     const struct macro_info *info;
> +     const cpp_string *string;
> +{
> +  const U_CHAR *p, *q, *limit = string->text + string->len;
> +  const cpp_token *token;

I'd rather you put the initialization of 'limit' just before the
loop.  It gets lost up there.  (This may be less of a problem after
you take out the debugging prints - use your judgement.

...
> +      /* Find the start of an identifier.  */
> +      while (!is_idstart(*p))
> +	p++;

Please write this

 while (!is_idstart (*p)) p++;

...
> +	  cpp_hashnode * hn = token->val.node;
> +	  
> +	  /* Skip the commas in between the arguments.  */
> +	  if (token->type != CPP_NAME)
> +	    continue;

Reverse the order of these.  token->val.node is an invalid pointer for
COMMA tokens.  I know it isn't used, but let's not touch it anyway.

> +	  cpp_warning (pfile, "checking string subtoken `%.*s' vs arg `%.*s'",
> +		       q - p, p, hn->length, hn->name);
> +	  if (hn->length == (unsigned long) (q - p)

Why is this cast necessary?

zw