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

[Jason Merrill <jason@redhat.com>]

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

> +/* 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.

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

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

> +/* 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.

Jason