On 7/6/22 10:23, Lewis Hyatt wrote:
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 <ja...@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.
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.
+/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
+ directly from libcpp. We also need to inform the token streamer about all
+ tokens we lex ourselves here, so it outputs them too; this is done by
calling
+ c_pp_stream_token () for each. */
Let's add to this comment
??? If we need to support more pragmas in the future, maybe initialize
this_parser with the pragma tokens and call pragma_lex?
OK with that change.
+static void
+pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
