On Fri, Feb 10, 2023 at 11:58:03AM -0500, Lewis Hyatt wrote:
> On Fri, Feb 10, 2023 at 11:30 AM Jakub Jelinek <ja...@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?
It has been posted during stage 1, on the other side we are already almost a
month into stage 4, and fixes an important bug which isn't a regression
though. I'd defer to the reviewers to decide that.
I've noticed this when seeing PR108717 being marked as dup of PR103902.
> 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.
I agree, I just wanted to mention that if both of the callers need the same
large comment and roughly or completely the same code that guards the
cpp_warning, then it is a good candidate for a new helper, exactly like
you've added for the large duplication in the first new function.
Jakub
