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

Jason Merrill jason@redhat.com
Sat Jul 2 00:23:53 GMT 2022


On 7/1/22 18:05, Lewis Hyatt wrote:
> 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.

Or creates the_parser itself and feeds it tokens somewhat like 
cp_parser_handle_statement_omp_attributes.

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

Sure.

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

Sounds good.

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



More information about the Gcc-patches mailing list