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
