Re: [patch] support for -fdump-ada-spec

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Sat, 1 May 2010, Arnaud Charlet wrote:

>  -x @var{language}  -v  -###  --help@r{[}=@var{class}@r{[},@dots{}@r{]]}  --target-help  @gol
> ---version -wrapper@@@var{file} -fplugin=@var{file} -fplugin-arg-@var{name}=@var{arg}}
> +--version -wrapper@@@var{file} -fplugin=@var{file} -fplugin-arg-@var{name}=@var{arg}  @gol
> +-fdump-ada-spec[-slim]}

Unless the [] are literally part of the option name, they should be @r{[} 
and @r{]} so they are not typeset in a fixed-width font.

> +Ada spec (via the -fdump-ada-spec switch).

@option{-fdump-ada-spec}.

> +@item -fdump-ada-spec[-slim]

Same point as above on [].

> +/* Dump all digits/hex chars from NUMBER to BUFFER.
> +   Returns a pointer to the character after the last character written.  */
> +
> +static unsigned char *
> +dump_number (unsigned char *number, unsigned char *buffer)
> +{
> +  while (*number != '\0' && *number != 'U' && *number != 'l' && *number != 'L')
> +    *buffer++ = *number++;

You may as well handle 'u' as well here, even if you don't want to handle 
hex floats and "float" literals ending 'f' or 'F'.

> +	      if (token->flags & STRINGIFY_ARG || token->flags & PASTE_LEFT)
> +		{
> +		  supported = 0;

Weren't those cases detected earlier as unsupported when you calculated 
the length?

> +		  case CPP_NOT:
> +		    *buffer++ = 'n'; *buffer++ = 'o'; *buffer++ = 't'; break;
> +		  case CPP_MOD:
> +		    *buffer++ = 'm'; *buffer++ = 'o'; *buffer++ = 'd'; break;
> +		  case CPP_AND:
> +		    *buffer++ = 'a'; *buffer++ = 'n'; *buffer++ = 'd'; break;
> +		  case CPP_OR:
> +		    *buffer++ = 'o'; *buffer++ = 'r'; break;
> +		  case CPP_XOR:
> +		    *buffer++ = 'x'; *buffer++ = 'o'; *buffer++ = 'r'; break;
> +		  case CPP_AND_AND:
> +		    strcpy ((char *) buffer, "and then");
> +		    buffer += 8;
> +		    break;
> +		  case CPP_OR_OR:
> +		    strcpy ((char *) buffer, "or else");
> +		    buffer += 7;
> +		    break;

Do you need to insert spaces before and after these strings?  The C tokens 
could be adjacent to identifiers with no spaces in between, but you don't 
want "a&b" to become "aandb".

> +		  case CPP_WSTRING:
> +		  case CPP_STRING16:
> +		  case CPP_STRING32:
> +		  case CPP_UTF8STRING:
> +		  case CPP_WCHAR:
> +		  case CPP_CHAR16:
> +		  case CPP_CHAR32:
> +		  case CPP_NAME:
> +		    if (!macro->fun_like)
> +		      supported = 0;
> +		    else
> +		      buffer = cpp_spell_token (parse_in, token, buffer, false);
> +		    break;
> +
> +		  case CPP_STRING:
> +		    is_string = 1;
> +		    buffer = cpp_spell_token (parse_in, token, buffer, false);
> +		    break;
> +
> +		  case CPP_CHAR:
> +		    is_char = 1;
> +		    buffer = cpp_spell_token (parse_in, token, buffer, false);
> +		    break;
> +
> +		  case CPP_NUMBER:

Although better than the previous code, you'd still get correct results 
for more cases if you interpret the value of the token rather than the 
spelling - for example, that way you could handle escape sequences 
appearing in strings and convert them appropriately for Ada without any 
need to know about the exact semantics of C escape sequences.

> +			      case '\0':
> +			      case 'l':
> +			      case 'L':
> +			      case 'U':

And 'u', similarly.

> +			      case 'b':

And 'B'.

> +			case '1':
> +			  if (tmp[1] == '\0' || tmp[1] == 'l'
> +			      || tmp[1] == 'L' || tmp [1] == 'U')

And 'u'.

> +		  default:
> +		    if (!macro->fun_like)
> +		      supported = 0;

I'm not clear on the logic of saying these things are supported for 
function-like macros but not otherwise.  Are there actually cases where 
respelling these C tokens will produce valid Ada, or should these tokens 
just make the whole macro expansion unsupported for Ada, or are you 
hoping a partial conversion, though not valid Ada, will still be useful 
to the users of this feature?

> +  macros = XALLOCAVEC (cpp_hashnode *, sizeof (cpp_hashnode *)*max_ada_macros);

XALLOCAVEC already does the multiplication by the size of the type passed, 
so it seems suspicious for the second argument to this call to be doing 
the multiplication itself.

I note that there are no testcases included with this patch - it would 
seem desirable to have testsuite coverage for this feature.  Practically 
this may mean having input files that provide reasonable coverage of the 
various cases covered in the code (for the whole feature, not just macros) 
and associated expected output files for comparison, though to avoid 
having to update lots of output files (or lots of cases in one big output 
file) when making minor changes to the output it would be better in 
principle to test not the output text but that it is correct Ada that 
passes through the Ada compiler and declares things in the expected way 
for Ada code using the generated declarations.

-- 
Joseph S. Myers
joseph@codesourcery.com