[PATCH] diagnostics: Add new option -fdiagnostics-plain-output

[Richard Sandiford]

Lewis Hyatt <lhyatt@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