From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x136.google.com (mail-lf1-x136.google.com [IPv6:2a00:1450:4864:20::136]) by sourceware.org (Postfix) with ESMTPS id 1FF143858C51 for ; Fri, 1 Jul 2022 22:06:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1FF143858C51 Received: by mail-lf1-x136.google.com with SMTP id e12so5990286lfr.6 for ; Fri, 01 Jul 2022 15:06:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=OV48N+Pz6wNFZXSsgEH8PPEACBZuJEuAVT1IwXqGFIQ=; b=14NNpA5VR13WjPL+xZuaaMPC3yOszEbL45gXCuTkkuAodF2O5edVkDwdtoDmD9q3Gx dK/ugQ08Menk0HomLdBClhEPTUVLRsIXBvNPLOhHS9bpvYoRQ1ZifklQg9o0/d/vinPb 8NkUI392JedQopYW9lOYBlj/ZIsxriuriaqLT/zgfxrj98K4xeNCuSy9FBIL0eLlKt/O 1OKQ6QEHbJXmMNw8YKE74teyd9s4dT08Pdit/pmSgXCpEmvrXuo4YKV5SPufx+5atZx4 czuIcPAdDK+CCWbPYE59KuEvMW4I9TQUjDKjtLCL1zjhv1WjWqzkA3lPTQ+NAeb4XiQr bJ4g== X-Gm-Message-State: AJIora8sER/rEk00JE3npmA2rQ9J5wibckLCLG1G6XWwbpj4gguXDrYI rsTOUx5rsUxHS+hHAaQ34TR+JkrnDF6JbATgr3U= X-Google-Smtp-Source: AGRyM1te2EjoFDchn9CejNiSB3t0GhaOr+YN77kSeR53MwZ0ODDmvCe0xeAp3bG08vzP9/mqyyVVxjG5t0o7I7Se9Gk= X-Received: by 2002:ac2:495d:0:b0:47f:a2bc:762c with SMTP id o29-20020ac2495d000000b0047fa2bc762cmr10124818lfi.93.1656713169362; Fri, 01 Jul 2022 15:06:09 -0700 (PDT) MIME-Version: 1.0 References: <0b17242b-1077-dc34-f7a1-cc93f854d004@redhat.com> In-Reply-To: <0b17242b-1077-dc34-f7a1-cc93f854d004@redhat.com> From: Lewis Hyatt Date: Fri, 1 Jul 2022 18:05:58 -0400 Message-ID: Subject: Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431] To: Jason Merrill Cc: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3029.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Jul 2022 22:06:13 -0000 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. 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