Re: Ping^3: [PATCH] libcpp: Handle extended characters in user-defined literal suffix [PR103902]

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Sep 26, 2022 at 06:27:25PM -0400, Lewis Hyatt via Gcc-patches wrote:
> May I please ping this patch again? Joseph suggested that it would be best if
> a C++ maintainer has a look at it. This is one of just a few places left where
> we don't handle UTF-8 properly in libcpp, it would be really nice to get them
> fixed up if there is time to review this patch. Thanks!

CCing them.

Just some nits from me, but I agree C++ maintainers are the best reviewers
for this.

> libcpp/ChangeLog:
> 
> 	PR preprocessor/103902
> 	* lex.cc (identifier_diagnostics_on_lex): New function refactors
> 	common code from...
> 	(lex_identifier_intern): ...here, and...
> 	(lex_identifier): ...here.
> 	(struct scan_id_result): New struct to hold the result of...

I'd just write
	(struct scan_id_result): New type.
or similar, no need to explain what it will be used for.

> 	(scan_cur_identifier): ...new function.

So just New function here too.

> 	(create_literal2): New function.
> 	(is_macro): Removed function that is now handled directly in
> 	lex_string() and lex_raw_string().
> 	(is_macro_not_literal_suffix): Likewise.
> 	(lit_accum::create_literal2): New function.
> 	(lex_raw_string): Make use of new function scan_cur_identifier().
> 	(lex_string): Likewise.

> +/* Helper function to perform diagnostics that are needed (rarely)
> +   when an identifier is lexed.  */
> +static void identifier_diagnostics_on_lex (cpp_reader *pfile,
> +					   cpp_hashnode *node)

Formatting, function name should be at the start of line, so
static void
identifier_diagnostics_on_lex (cpp_reader *pfile, cpp_hashnode *node)

> +{
> +  if (__builtin_expect (!(node->flags & NODE_DIAGNOSTIC)
> +			|| pfile->state.skipping, 1))
> +    return;
> +
> +  /* It is allowed to poison the same identifier twice.  */
> +  if ((node->flags & NODE_POISONED) && !pfile->state.poisoned_ok)
> +    cpp_error (pfile, CPP_DL_ERROR, "attempt to use poisoned \"%s\"",
> +	       NODE_NAME (node));
> +
> +  /* Constraint 6.10.3.5: __VA_ARGS__; should only appear in the
> +     replacement list of a variadic macro.  */
> +  if (node == pfile->spec_nodes.n__VA_ARGS__
> +      && !pfile->state.va_args_ok)
> +    {
> +      if (CPP_OPTION (pfile, cplusplus))
> +	cpp_error (pfile, CPP_DL_PEDWARN,
> +		   "__VA_ARGS__ can only appear in the expansion"
> +		   " of a C++11 variadic macro");
> +      else
> +	cpp_error (pfile, CPP_DL_PEDWARN,
> +		   "__VA_ARGS__ can only appear in the expansion"
> +		   " of a C99 variadic macro");
> +    }
> +

Perhaps add here the:
+      /* __VA_OPT__ should only appear in the replacement list of a
+	 variadic macro.  */
comment that used to be present only in the second occurrence of
all this.

> +  if (node == pfile->spec_nodes.n__VA_OPT__)
> +    maybe_va_opt_error (pfile);
> +
> +  /* For -Wc++-compat, warn about use of C++ named operators.  */
> +  if (node->flags & NODE_WARN_OPERATOR)
> +    cpp_warning (pfile, CPP_W_CXX_OPERATOR_NAMES,
> +		 "identifier \"%s\" is a special operator name in C++",
> +		 NODE_NAME (node));
> +}
> +
> +/* Helper function to scan an entire identifier beginning at
> +   pfile->buffer->cur, and possibly containing extended characters (UCNs
> +   and/or UTF-8).  Returns the cpp_hashnode for the identifier on success, or
> +   else nullptr, as well as a normalize_state so that normalization warnings
> +   may be issued once the token lexing is complete.  */

This looks like a function comment that should be immediately above
scan_cur_identifier, there might be another comment above struct
scan_id_result which would just explain the purpose of the class.

> +
> +struct scan_id_result
> +{
> +  cpp_hashnode *node;
> +  normalize_state nst;
> +
> +  scan_id_result ()
> +    : node (nullptr)
> +  {
> +    nst = INITIAL_NORMALIZE_STATE;
> +  }
> +
> +  explicit operator bool () const { return node; }
> +};
> +
> +static scan_id_result
> +scan_cur_identifier (cpp_reader *pfile)
> +{

> @@ -2741,26 +2800,53 @@ lex_raw_string (cpp_reader *pfile, cpp_token *token, const uchar *base)
>  
>    if (CPP_OPTION (pfile, user_literals))
>      {
> -      /* If a string format macro, say from inttypes.h, is placed touching
> -	 a string literal it could be parsed as a C++11 user-defined string
> -	 literal thus breaking the program.  */
> -      if (is_macro_not_literal_suffix (pfile, pos))
> -	{
> -	  /* Raise a warning, but do not consume subsequent tokens.  */
> -	  if (CPP_OPTION (pfile, warn_literal_suffix) && !pfile->state.skipping)
> -	    cpp_warning_with_line (pfile, CPP_W_LITERAL_SUFFIX,
> -				   token->src_loc, 0,
> -				   "invalid suffix on literal; C++11 requires "
> -				   "a space between literal and string macro");
> -	}
> -      /* Grab user defined literal suffix.  */
> -      else if (ISIDST (*pos))
> -	{
> -	  type = cpp_userdef_string_add_type (type);
> -	  ++pos;
> +      const uchar *const suffix_begin = pos;
> +      pfile->buffer->cur = pos;
>  
> -	  while (ISIDNUM (*pos))
> -	    ++pos;
> +      if (const auto sr = scan_cur_identifier (pfile))
> +	{
> +	  /* If a string format macro, say from inttypes.h, is placed touching
> +	     a string literal it could be parsed as a C++11 user-defined
> +	     string literal thus breaking the program.  User-defined literals
> +	     outside of namespace std must start with a single underscore, so
> +	     assume anything of that form really is a UDL suffix.  We don't
> +	     need to worry about UDLs defined inside namespace std because
> +	     their names are reserved, so cannot be used as macro names in
> +	     valid programs.  */
> +	  if ((suffix_begin[0] != '_' || suffix_begin[1] == '_')
> +	      && cpp_macro_p (sr.node))

What is the advantage of dropping is_macro_not_literal_suffix and
hand-inlining it in two different spots?
Couldn't even the actual warning be moved into an inline function?

> +	    {
> +	      /* Maybe raise a warning, but do not consume the tokens.  */
> +	      pfile->buffer->cur = suffix_begin;
> +	      if (CPP_OPTION (pfile, warn_literal_suffix)
> +		  && !pfile->state.skipping)
> +		cpp_warning_with_line
> +		  (pfile, CPP_W_LITERAL_SUFFIX,
> +		   token->src_loc, 0,
> +		   "invalid suffix on literal; C++11 requires "
> +		   "a space between literal and string macro");

The ( on a call on a different line is quite ugly, so if it can be avoided,
it should.
		cpp_warning_with_line (pfile, CPP_W_LITERAL_SUFFIX,
				       token->src_loc, 0,
				       "invalid suffix on literal; C++11 "
				       "requires a space between literal "
				       "and string macro");
is more readable and same number of lines.

Otherwise it looks reasonable to me, but I'd still prefer Jason or Nathan
to review this.

	Jakub