Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Lewis Hyatt <lhyatt@gmail.com>]

On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill <jason@redhat.com> wrote:
>
> On 6/29/22 12:59, Jason Merrill wrote:
> > On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
> >> Hello-
> >>
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
> >>
> >> Would a C++ maintainer have some time to take a look at this patch
> >> please? I feel like the PR is still worth resolving. If this doesn't
> >> seem like a good way, I am happy to try another -- would really
> >> appreciate any feedback. Thanks!
> >
> > Thanks for your persistence, I'll take a look now.
> >
> > Incidentally, when pinging it's often useful to ping someone from
> > MAINTAINERS directly, as well as the list.  I think your last ping got
> > eaten by some trouble Red Hat email was having at the time.
> >
> > The cp_token_is_module_directive cleanup is OK.

Thank you very much for the advice and for going through the patch, I
really appreciate it. I went ahead and pushed the small cleanup patch.
I have responses to your comments on the main patch below too.

> >
> >> +  bool skip_this_pragma;
> >
> > This member seems to be equivalent to
> >   in_pragma && !should_output_pragmas ()
> > Maybe it could be a member function instead of a data member?
> >

Yeah, makes sense, although I hope that by implementing your
suggestion below regarding rewinding the tokens for preprocessing,
then this can be removed anyway.

> > More soon.
>
> Looks good, just a few minor comments:
>
> > +      PD_KIND_INVALID,
> > +      PD_KIND_PUSH,
> > +      PD_KIND_POP,
> > +      PD_KIND_IGNORED_ATTRIBUTES,
> > +      PD_KIND_DIAGNOSTIC,
>
> The PD_KIND_ prefix seems unnecessary for a scoped enum.
>

Sure, will shorten it to PK_ instead.

> > +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> > +   directly from libcpp.  */
> > +static void
> > +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
>
> Hmm, we could make a temporary lexer, but I guess this is short enough
> that the duplication is OK.
>

I see. It would look like a version of pragma_lex() (the one in
c-parser.cc) which took a c_parser* argument so it wouldn't need to
use the global the_parser. I didn't consider this because I was
starting from Manuel's prototype patch on the PR
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c10), which was
doing the parsing in libcpp itself. Perhaps it would make sense to
move to this approach in the future, if it became necessary sometime
to lex some other pragmas during preprocessing?

> > +/* Similar, for the portion of a diagnostic pragma that was parsed
> > +   internally and so not seen by our token streamer.  */
>
> Can we rewind after parsing so that the token streamer sees it?
>

Oh that's an interesting idea. It would avoid some potential issues.
For instance, I have another patch submitted to fix PR55971
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55971#c8), which is that
you can't put raw strings containing newlines into a preprocessing
directive. It occurs to me now, once that's applied, then I think a
#pragma GCC diagnostic with such a raw string (useless though it would
be) would not get output correctly by gcc -E with this current patch's
approach. An edge case certainly, but would be nice to get it right
also, and your approach would automatically handle it. I'm going to
explore this now and then follow up with a new version of the patch.

> > +  if (early && arg)
> > +    {
> > +      /* This warning is needed because of PR71603 - popping the diagnostic
> > +        state does not currently reset changes to option arguments, only
> > +        changes to the option dispositions.  */
> > +      warning_at (data.loc_option, OPT_Wpragmas,
> > +                 "a diagnostic pragma attempting to modify a preprocessor"
> > +                 " option argument may not work as expected");
> > +    }
>
> Maybe only warn if we subsequently see a pop?
>

Right, that makes sense. Changing the option does work just fine until
you try to pop it. But actually this warning was kinda an
afterthought, now I just checked and at the time I wrote it, there was
only one option it could possibly apply to, since it needs to be an
option that's both for libcpp, and taking an argument, which was
-Wnormalized=. In the meantime one more has been added, -Wbidi-chars=.
Rather than add more complicated logic to remember to warn on pop for
these 2 options only, feels like maybe it would be better to either
just fix PR71603 (which I can work on sometime), or add this warning
for all options, not just libcpp options, which I guess means it
should go inside the implementation of pop... So in either case feels
like it's not really relevant to this patch and I'd propose just to
remove it for now, and then address it subsequently?

> > +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
> > +   for the case of preprocessing-related diagnostics.  */
> > +static void
> > +cp_lexer_handle_early_pragma (cp_lexer *lexer)
>
> Let's mention in the comment that this is called before appending the
> CPP_PRAGMA_EOL to the lexer buffer.

Will do.

Thanks again, I will work on a new version and reply to this thread soon.


-Lewis