Re: [RFA] Implement __VA_OPT__

[Alexander Monakov <amonakov at ispras dot ru>]

On Sat, 16 Sep 2017, Tom Tromey wrote:
> --- a/gcc/doc/cpp.texi
> +++ b/gcc/doc/cpp.texi
> @@ -1675,20 +1675,27 @@ macro.  We could define @code{eprintf} like this, instead:
[snip]
> +This formulation looks more descriptive, but historically it was less
> +flexible: you had to supply at least one argument after the format
> +string.  In standard C, you could not omit the comma separating the
> +named argument from the variable arguments.  (Note that this
> +restriction has been lifted in C++20, and never existed in GNU C; see
> +below.)

Shouldn't this say 'C++2a' (more instances follow)?


> +Historically, GNU has also had another extension to handle the

'GNU CPP'?

> --- a/libcpp/init.c
> +++ b/libcpp/init.c
> @@ -91,28 +91,29 @@ struct lang_flags
>  static const struct lang_flags lang_defaults[] =
> -{ /*              c99 c++ xnum xid c11 std digr ulit rlit udlit bincst digsep trig u8chlit */
[snip]
> -  /* GNUCXX17 */  { 1,  1,  1,  1,  1,  0,  1,   1,   1,   1,    1,     1,     0,   1 },
> -  /* CXX17    */  { 1,  1,  1,  1,  1,  1,  1,   1,   1,   1,    1,     1,     0,   1 },
> -  /* GNUCXX2A */  { 1,  1,  1,  1,  1,  0,  1,   1,   1,   1,    1,     1,     0,   1 },
> -  /* CXX2A    */  { 1,  1,  1,  1,  1,  1,  1,   1,   1,   1,    1,     1,     0,   1 },
> -  /* ASM      */  { 0,  0,  1,  0,  0,  0,  0,   0,   0,   0,    0,     0,     0,   0 }
> +{ /*              c99 c++ xnum xid c11 std digr ulit rlit udlit bincst digsep trig u8chlit vaopt */
[snip]
> +  /* GNUCXX1Z */  { 1,  1,  1,  1,  1,  0,  1,   1,   1,   1,    1,     1,     0,   1,      0 },
> +  /* CXX1Z    */  { 1,  1,  1,  1,  1,  1,  1,   1,   1,   1,    1,     1,     0,   1,      0 },
> +  /* GNUCXX2A */  { 1,  1,  1,  1,  1,  0,  1,   1,   1,   1,    1,     1,     0,   1,      1 },
> +  /* CXX2A    */  { 1,  1,  1,  1,  1,  1,  1,   1,   1,   1,    1,     1,     0,   1,      1 },
> +  /* ASM      */  { 0,  0,  1,  0,  0,  0,  0,   0,   0,   0,    0,     0,     0,   0,      0 }
>  };

This hunk reverts CXX17 back to CXX1Z.

> --- a/libcpp/lex.c
> +++ b/libcpp/lex.c
> @@ -1396,6 +1396,21 @@ lex_identifier_intern (cpp_reader *pfile, const uchar *base)
>  		       " of a C99 variadic macro");
>  	}
>  
> +      /* __VA_OPT__ should only appear in the replacement list of a
> +	 variadic macro.  */
> +      if (result == pfile->spec_nodes.n__VA_OPT__
> +	  && !pfile->state.va_args_ok)
> +	{
> +	  if (CPP_OPTION (pfile, cplusplus))
> +	    cpp_error (pfile, CPP_DL_ERROR,
> +		       "__VA_OPT__ can only appear in the expansion"
> +		       " of a C++11 variadic macro");
> +	  else
> +	    cpp_error (pfile, CPP_DL_ERROR,
> +		       "__VA_OPT__ can only appear in the expansion"
> +		       " of a C99 variadic macro");
> +	}
> +
>        /* For -Wc++-compat, warn about use of C++ named operators.  */
>        if (result->flags & NODE_WARN_OPERATOR)
>  	cpp_warning (pfile, CPP_W_CXX_OPERATOR_NAMES,
> @@ -1485,6 +1500,21 @@ lex_identifier (cpp_reader *pfile, const uchar *base, bool starts_ucn,
>  		       " of a C99 variadic macro");
>  	}
>  
> +      /* __VA_OPT__ should only appear in the replacement list of a
> +	 variadic macro.  */
> +      if (result == pfile->spec_nodes.n__VA_OPT__
> +	  && !pfile->state.va_args_ok)
> +	{
> +	  if (CPP_OPTION (pfile, cplusplus))
> +	    cpp_error (pfile, CPP_DL_ERROR,
> +		       "__VA_OPT__ can only appear in the expansion"
> +		       " of a C++11 variadic macro");
> +	  else
> +	    cpp_error (pfile, CPP_DL_ERROR,
> +		       "__VA_OPT__ can only appear in the expansion"
> +		       " of a C99 variadic macro");
> +	}
> +
>        /* For -Wc++-compat, warn about use of C++ named operators.  */
>        if (result->flags & NODE_WARN_OPERATOR)
>  	cpp_warning (pfile, CPP_W_CXX_OPERATOR_NAMES,

These two hunks add more duplication in already-duplicated 'if' statement
bodies.  Is it possible to move the whole body to a separate function?

I guess the references to C++11 and C99 are duplicated from the preceding 'if',
but is that correct here?  And why is it useful to distinguish between C and C++
for this error in the first place, why not simply say 'cpp_error (...,
"__VA_OPT__ can only appear in the expansion of a variadic macro");'?

> --- a/libcpp/macro.c
> +++ b/libcpp/macro.c
> @@ -1670,6 +1821,8 @@ replace_args (cpp_reader *pfile, cpp_hashnode *node, cpp_macro *macro,
>  				 num_macro_tokens);
>      }
>    i = 0;
> +  vaopt_state state_tracker (pfile, macro->variadic,
> +			     args[macro->paramc - 1].count > 0);

The name 'state_tracker' seems too general for what it does, I think
'vaopt_st' or such would have been a better fit (sorry for the bikeshed).

Thanks.
Alexander