On Fri, Jul 01, 2022 at 08:23:53PM -0400, Jason Merrill wrote: > On 7/1/22 18:05, Lewis Hyatt wrote: > > On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill 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. > > > Here is an updated patch addressing all your comments, thanks again for the review. Regarding the idea about rewinding tokens, it seems potentially problematic to try to make use of libcpp's backup token feature. I think there can be pragmas which allow macro expansion, and then libcpp is not able to step back through the expansion AFAIK. It would work fine for `#pragma GCC diagnostic' but would break if a similar approach was added for some other pragma allowing expansion in the future, and I think potentially also for _Pragma. But it seems pretty tenable to handle this case just by providing an interface in c-ppoutput.c to ask it to stream a given token that you have lexed external to the token streaming process. That's how I set it up here, it's much better than my first way IMHO, hopefully it looks OK? I also attached a diff of this version vs the previous version in case that makes it easier to review. The C++ parts were not touched other than adding the comment you suggested, the changes in this version are mostly in c-ppoutput.cc and c-pragma.cc. Thanks again. -Lewis