From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 891FD3858D38 for ; Wed, 12 Aug 2020 16:54:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 891FD3858D38 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3C3A031B; Wed, 12 Aug 2020 09:54:24 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A07BB3F22E; Wed, 12 Aug 2020 09:54:23 -0700 (PDT) From: Richard Sandiford To: Lewis Hyatt Mail-Followup-To: Lewis Hyatt , gcc-patches@gcc.gnu.org, David Malcolm , richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, David Malcolm Subject: Re: [PATCH] diagnostics: Add new option -fdiagnostics-plain-output References: <20200811143418.GA820@ldh-imac.local> Date: Wed, 12 Aug 2020 17:54:21 +0100 In-Reply-To: <20200811143418.GA820@ldh-imac.local> (Lewis Hyatt's message of "Tue, 11 Aug 2020 10:34:18 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 12 Aug 2020 16:54:26 -0000 Lewis Hyatt writes: > Hello- > > Attached is the patch I mentioned in another discussion here: > https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551442.html > > This adds a new option -fdiagnostics-plain-output that currently means the > same thing as: > -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers > -fdiagnostics-color=3Dnever -fdiagnostics-urls=3Dnever > > The idea is that over time, if diagnostics output changes to get more bel= ls > and whistles by default (such as the UTF-8 output I suggested in the above > discussion), -fdiagnostics-plain-output will adjust to turn that back off, > so that the testsuite needs only the one option and doesn't need to get > updated every time something new is added. It seems to me that when > diagnostics change, it's otherwise a bit hard to update the testsuite > correctly, especially for compat.exp that is often not run by default. I > think this would also be useful for utilities that want to parse the > diagnostics (but aren't using -fdiagnostics-output-format=3Djson). > > BTW, I considered avoiding adding a new switch by having this option take > the form -fdiagnostics-output-format=3Dplain instead, but this seems to h= ave > problematic semantics when multiple related options are specified. Given = that > this option needs to be expanded early in the parsing process, so that it > can be compatible with the special handling for -fdiagnostics-color, it > seemed best to just make it a simple option with no negated form. > > I hope this may be useful, please let me know if you'd like me to push > it. bootstrap and regtest were done for all languages on x86-64 Linux, all > tests the same before and after, and same for the compat.exp with > alternate compiler GCC 8.2. Thanks for doing this. LGTM except for a couple of very minor things: > [=E2=80=A6] > @@ -981,6 +982,42 @@ decode_cmdline_options_to_array (unsigned int argc, = const char **argv, > argv[++i] =3D replacement; > } >=20=20 > + /* Expand -fdiagnostics-plain-output to its constituents. This ne= eds > + to happen here so that prune_options can handle -fdiagnostics-color > + specially. */ > + if (!strcmp (opt, "-fdiagnostics-plain-output")) > + { > + /* If you have changed the default diagnostics output, and this new > + output is not appropriately "plain" (e.g., the change needs to be > + undone in order for the testsuite to work properly), then please do > + the following: With GCC formatting, the paragraph should be indented under =E2=80=9CIf you= =E2=80=A6=E2=80=9D. > + 1. Add the necessary option to undo the new behavior to > + the array below. > + 2. Update the documentation for -fdiagnostics-plain-output > + in invoke.texi. > + */ =E2=80=A6and this should be on the previous line (=E2=80=9C. */=E2=80=9D). > + const char *const expanded_args[] =3D { > + "-fno-diagnostics-show-caret", > + "-fno-diagnostics-show-line-numbers", > + "-fdiagnostics-color=3Dnever", > + "-fdiagnostics-urls=3Dnever", > + }; Hadn't expected it to be this complicated :-) But I agree with your reasoning: it looks like this is the correct way given the special handling of -fdiagnostic-color (and potentially other -fdiagnostic options in future). > + const int num_expanded > + =3D sizeof expanded_args / sizeof (*expanded_args); Simplifies to =E2=80=9CARRAY_SIZE (expanded_args)=E2=80=9D. > + opt_array_len +=3D num_expanded - 1; > + opt_array =3D XRESIZEVEC (struct cl_decoded_option, > + opt_array, opt_array_len); > + for (int j =3D 0; j < num_expanded;) > + { > + j +=3D decode_cmdline_option (expanded_args + j, lang_mask, > + &opt_array[num_decoded_options]); Might be worth using the same "for" loop structure as the outer loop, assigning the number of decoded options to =E2=80=9Cn=E2=80=9D. Neither's = better than the other, but it makes it clearer that there's nothing special going on. > + num_decoded_options++; > + } > + > + n =3D 1; > + continue; > + } > + > n =3D decode_cmdline_option (argv + i, lang_mask, > &opt_array[num_decoded_options]); > num_decoded_options++; > diff --git a/gcc/testsuite/lib/c-compat.exp b/gcc/testsuite/lib/c-compat.= exp > index 9493c214aea..5f43c5a6a57 100644 > --- a/gcc/testsuite/lib/c-compat.exp > +++ b/gcc/testsuite/lib/c-compat.exp > @@ -36,24 +36,34 @@ load_lib target-libpath.exp > proc compat-use-alt-compiler { } { > global GCC_UNDER_TEST ALT_CC_UNDER_TEST > global compat_same_alt compat_alt_caret compat_alt_color compat_no_l= ine_no > - global compat_alt_urls > + global compat_alt_urls compat_alt_plain_output > global TEST_ALWAYS_FLAGS >=20=20 > # We don't need to do this if the alternate compiler is actually > # the same as the compiler under test. > if { $compat_same_alt =3D=3D 0 } then { > set GCC_UNDER_TEST $ALT_CC_UNDER_TEST > + > + #These flags are no longer added to TEST_ALWAYS_FLAGS by prune.exp > + #because they are subsumed by -fdiagnostics-plain-output. Add them back > + #for compatibility testing with older compilers that do not understand > + #-fdiagnostics-plain-output. I think the convention (in GCC) is to have a space after the =E2=80=9C#=E2= =80=9D. Same for the other comments. > + set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret -fno-diagnostics-sho= w-line-numbers -fdiagnostics-color=3Dnever -fdiagnostics-urls=3Dnever $TEST= _ALWAYS_FLAGS" I was initially surprised that the existing code didn't reset TEST_ALWAYS_FLAGS to compat_save_TEST_ALWAYS_FLAGS before altering it, but I guess there's no need if all it's doing is removing flags. (So I agree what the patch is doing is correct as-is.) OK with those changes in 24 hrs if noone objects or asks for a delay. Thanks, Richard