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

[Lewis Hyatt <lhyatt@gmail.com>]

On Fri, Feb 10, 2023 at 11:30 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> 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.

Thanks so much for looking it over, I really appreciate it. I'll be
sure to incorporate all your feedback along with those from the full
review.

Is this for stage 1 at this point BTW?

One note, the patch as-is doesn't quite apply to master branch
nowadays, it just needs a small tweak since warn_about_normalization()
has acquired a new argument in the meantime. If it's helpful I can
resend it with this addressed, as well as the rest of your comments?

Finally one comment here:

> > +      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?

The is_macro() function was doing two jobs, first lexing the
identifier and looking it up in the hash table, and then calling
cpp_macro_p(). This was a bit duplicative because the identifier was
then immediately lexed again after the check. Since lexing it became
more complicated with UTF-8 support, I changed it not to duplicate
that effort and instead scan_cur_identifer() does the job once. With
that done, all that's left for is_macro() to do is just the one line
check so I got rid of it. However, I agree that the check about
suffix_begin is not really trivial and so factoring this out into one
place instead of two makes sense. I'll try to move the whole warning
into its own function in the next iteration.

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

Thanks again.

-Lewis