On Wed, Aug 12, 2020 at 12:54 PM Richard Sandiford <richard.sandif...@arm.com> wrote: > > Lewis Hyatt <lhy...@gmail.com> 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=never -fdiagnostics-urls=never > > > > The idea is that over time, if diagnostics output changes to get more bells > > 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=json). > > > > BTW, I considered avoiding adding a new switch by having this option take > > the form -fdiagnostics-output-format=plain instead, but this seems to have > > 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: > > > […] > > @@ -981,6 +982,42 @@ decode_cmdline_options_to_array (unsigned int argc, > > const char **argv, > > argv[++i] = replacement; > > } > > > > + /* Expand -fdiagnostics-plain-output to its constituents. This needs > > + 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 “If you…”. > > > + 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. > > + */ > > …and this should be on the previous line (“. */”). > > > + const char *const expanded_args[] = { > > + "-fno-diagnostics-show-caret", > > + "-fno-diagnostics-show-line-numbers", > > + "-fdiagnostics-color=never", > > + "-fdiagnostics-urls=never", > > + }; > > 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 > > + = sizeof expanded_args / sizeof (*expanded_args); > > Simplifies to “ARRAY_SIZE (expanded_args)”. > > > + opt_array_len += num_expanded - 1; > > + opt_array = XRESIZEVEC (struct cl_decoded_option, > > + opt_array, opt_array_len); > > + for (int j = 0; j < num_expanded;) > > + { > > + j += 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 “n”. Neither's better than > the other, but it makes it clearer that there's nothing special going on. > > > + num_decoded_options++; > > + } > > + > > + n = 1; > > + continue; > > + } > > + > > n = 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_line_no > > - global compat_alt_urls > > + global compat_alt_urls compat_alt_plain_output > > global TEST_ALWAYS_FLAGS > > > > # We don't need to do this if the alternate compiler is actually > > # the same as the compiler under test. > > if { $compat_same_alt == 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 “#”. > Same for the other comments. > > > + set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret > > -fno-diagnostics-show-line-numbers -fdiagnostics-color=never > > -fdiagnostics-urls=never $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
Thanks for the review, and sorry about the formatting glitches. I pushed it just now with your fixes. -Lewis
