[clang][Driver] Add -fcaret-diagnostics-max-lines= as a driver option

[tbaeder]

Summary

Since https://reviews.llvm.org/D147875 landed, setting different values (or reverting to the old default of 1) is more important than before, so promote this option to a driver flag.

[hans]

Should there be a test?

[MaskRay]

def fcaret_diagnostics_max_lines in Options.td has the NoDriverOption flag. Move it to other places with BooleainFFlag should work.
Then in Clang.cpp you can just write Args.AddLastArg(...)

Can you add some description that this is related to D147875 ?

[tbaeder]

In D152090#4395827, @MaskRay wrote:

def fcaret_diagnostics_max_lines in Options.td has the NoDriverOption flag. Move it to other places with BooleainFFlag should work.
Then in Clang.cpp you can just write Args.AddLastArg(...)

Can you add some description that this is related to D147875 ?

Can you give a more concrete example? I modeled this patch after existing similar options, like -fmacro-backtrace-limit.

[aaron.ballman]

In D152090#4395192, @hans wrote:

Should there be a test?

+1

Also, a release note so users know about the public-facing option?

clang/include/clang/Driver/Options.td

1631

This should have HelpText as well, right?

[MaskRay]

In D152090#4398097, @tbaeder wrote:

In D152090#4395827, @MaskRay wrote:

def fcaret_diagnostics_max_lines in Options.td has the NoDriverOption flag. Move it to other places with BooleainFFlag should work.
Then in Clang.cpp you can just write Args.AddLastArg(...)

Can you add some description that this is related to D147875 ?

Can you give a more concrete example? I modeled this patch after existing similar options, like -fmacro-backtrace-limit.

Thanks for mentioning -fmacro-backtrace-limit. I changed it and a few similar options to the perfered form in 0aa4af711e567c8683cee95ffbb14812df03cecc

We have many legacy Separate form CC1 options that unnecessarily differ from their Joined (ended with =) driver option counterparts.
New code is recommended to use Flags<[CC1Option]>,.
As a general advice, if you see Separate, chances are that they likely don't use the best practice...

[MaskRay]

clang/include/clang/Driver/Options.td

2318

Add (0 = no limit) if useful

clang/test/Driver/caret-diagnostics-max-lines.cpp

2

The test/Driver test just tests that -fcaret-diagnostics-max-lines=2 is forwarded to CC1. The semantic tests are performed in other test directories, Misc/caret-diags-multiline.cpp in this case.

14

Delete the trailing blank line

[MaskRay]

Add -fcaret-diagnostics-max-lines as a driver option

-fcaret-diagnostics-max-lines= is more accurate.

[dyung]

If we do not accept the form "-fcaret-diagnostics-max-lines n", can we add a negative for that? Or if we do a positive test for it as well?

[tbaeder]

clang/test/Driver/caret-diagnostics-max-lines.cpp

2

Do you want me to remove the semantic part in this file or add a RUN line to caret-diags-multiline.cpp using %clang?

[MaskRay]

In D152090#4407632, @dyung wrote:

If we do not accept the form "-fcaret-diagnostics-max-lines n", can we add a negative for that? Or if we do a positive test for it as well?

I think this is not very useful. This is a new option. Separate-form driver options are unusual and typically legacy GCC options (before 2000 or early 2000s).
If we have tested -fcaret-diagnostics-max-lines=N, it is unnecessary to test -fcaret-diagnostics-max-lines N.

clang/test/Driver/caret-diagnostics-max-lines.cpp

2

clang/test/Driver/caret-diagnostics-max-lines.cpp should just check that the driver option -fcaret-diagnostics-max-lines=N is translated to -cc1 {{.*}}"-fcaret-diagnostics-max-lines=N".

The functionality feature is tested in other test directories, and should not be duplicated in test/Driver/.

[MaskRay]

LGTM.

clang/include/clang/Driver/Options.td

2318

NoXarchOption emits an error if an option is used after -Xarch_*. The feature only applies to a small set of options (e.g. -o) and is not very useful for most options, but NoXarchOption was improperly named DriverOption and lured some contributors to add NoXarchOption to options that should not have the flag.

For fcaret-diagnostics-max-lines=, we should remove NoXarchOption.

[tbaeder]

clang/include/clang/Driver/Options.td

2318

Removed it, thanks for the explanation. I guess this should be removed from -fmacro-backtrace-limit as well?

[TWeaver]

Hello and good afternoon from the UK,

It looks as though this change has caused the following buildbot to start failing (it's temporarily on staging server so you may not have seen it on the public build bot page):

https://lab.llvm.org/staging/#/builders/204/builds/2141

Are you able to take a look?

Thanks,
Tom W

[TWeaver]

Thanks for the fix! 👍

clang/docs/ReleaseNotes.rst

- Clang now support ``__builtin_FUNCSIG()`` which returns the same information
   This fixes (`#58951 <https://github.com/llvm/llvm-project/issues/58951>`_).
 - Clang now supports the `NO_COLOR <https://no-color.org/>`_ environment
   variable as a way to disable color diagnostics.
 
 New Compiler Flags
 ------------------
 - The flag ``-std=c++23`` has been added. This behaves the same as the existing
   flag ``-std=c++2b``.
-
 - ``-dumpdir`` has been implemented to specify auxiliary and dump output
   filenames for features like ``-gsplit-dwarf``.
+- ``-fcaret-diagnostics-max-lines=`` has been added as a driver options, which
+  lets users control the maximum number of source lines printed for a
+  caret diagnostic.
 
 Deprecated Compiler Flags
 -------------------------
 
 Modified Compiler Flags
 -----------------------
 
 - ``clang -g -gsplit-dwarf a.c -o obj/x`` (compile and link) now generates the

clang/include/clang/Driver/Options.td

def femit_all_decls : Flag<["-"], "femit-all-decls">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Emit all declarations, even if unused">,
   MarshallingInfoFlag<LangOpts<"EmitAllDecls">>;
 defm emulated_tls : BoolFOption<"emulated-tls",
   CodeGenOpts<"EmulatedTLS">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Use emutls functions to access thread_local variables">,
   NegFlag<SetFalse>>;
 def fencoding_EQ : Joined<["-"], "fencoding=">, Group<f_Group>;
 def ferror_limit_EQ : Joined<["-"], "ferror-limit=">, Group<f_Group>, Flags<[CoreOption]>;
 defm exceptions : BoolFOption<"exceptions",
   LangOpts<"Exceptions">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable">, NegFlag<SetFalse, [], "Disable">,
   BothFlags<[], " support for exception handling">>;
 def fdwarf_exceptions : Flag<["-"], "fdwarf-exceptions">, Group<f_Group>,
   HelpText<"Use DWARF style exceptions">;
 def fsjlj_exceptions : Flag<["-"], "fsjlj-exceptions">, Group<f_Group>,
   HelpText<"Use SjLj style exceptions">;
 def fseh_exceptions : Flag<["-"], "fseh-exceptions">, Group<f_Group>,

 def fthin_link_bitcode_EQ : Joined<["-"], "fthin-link-bitcode=">,
   Flags<[CoreOption, CC1Option]>, Group<f_Group>,
   HelpText<"Write minimized bitcode to <file> for the ThinLTO thin link only">,
   MarshallingInfoString<CodeGenOpts<"ThinLinkBitcodeFile">>;
 def fmacro_backtrace_limit_EQ : Joined<["-"], "fmacro-backtrace-limit=">,
   Group<f_Group>, Flags<[NoXarchOption, CC1Option, CoreOption]>,
   HelpText<"Set the maximum number of entries to print in a macro expansion backtrace (0 = no limit)">,
   MarshallingInfoInt<DiagnosticOpts<"MacroBacktraceLimit">, "DiagnosticOptions::DefaultMacroBacktraceLimit">;
+def fcaret_diagnostics_max_lines_EQ :
+  Joined<["-"], "fcaret-diagnostics-max-lines=">,
+  Group<f_Group>, Flags<[CC1Option, CoreOption]>,
+  HelpText<"Set the maximum number of source lines to show in a caret diagnostic (0 = no limit).">,
+  MarshallingInfoInt<DiagnosticOpts<"SnippetLineLimit">, "DiagnosticOptions::DefaultSnippetLineLimit">;
 defm merge_all_constants : BoolFOption<"merge-all-constants",
   CodeGenOpts<"MergeAllConstants">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option, CoreOption], "Allow">, NegFlag<SetFalse, [], "Disallow">,
   BothFlags<[], " merging of constants">>;
 def fmessage_length_EQ : Joined<["-"], "fmessage-length=">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Format message diagnostics so that they fit within N columns">,
   MarshallingInfoInt<DiagnosticOpts<"MessageLength">>;
 def frandomize_layout_seed_EQ : Joined<["-"], "frandomize-layout-seed=">,
def fno_diagnostics_use_presumed_location : Flag<["-"], "fno-diagnostics-use-presumed-location">,
   HelpText<"Ignore #line directives when displaying diagnostic locations">,
   MarshallingInfoNegativeFlag<DiagnosticOpts<"ShowPresumedLoc">>;
 def ftabstop : Separate<["-"], "ftabstop">, MetaVarName<"<N>">,
   HelpText<"Set the tab stop distance.">,
   MarshallingInfoInt<DiagnosticOpts<"TabStop">, "DiagnosticOptions::DefaultTabStop">;
 def ferror_limit : Separate<["-"], "ferror-limit">, MetaVarName<"<N>">,
   HelpText<"Set the maximum number of errors to emit before stopping (0 = no limit).">,
   MarshallingInfoInt<DiagnosticOpts<"ErrorLimit">>;
-def fcaret_diagnostics_max_lines :
-  Separate<["-"], "fcaret-diagnostics-max-lines">, MetaVarName<"<N>">,
-  HelpText<"Set the maximum number of source lines to show in a caret diagnostic">,
-  MarshallingInfoInt<DiagnosticOpts<"SnippetLineLimit">, "DiagnosticOptions::DefaultSnippetLineLimit">;
 def verify_EQ : CommaJoined<["-"], "verify=">,
   MetaVarName<"<prefixes>">,
   HelpText<"Verify diagnostic output using comment directives that start with"
            " prefixes in the comma-separated sequence <prefixes>">;
 def verify : Flag<["-"], "verify">,
   HelpText<"Equivalent to -verify=expected">;
 def verify_ignore_unexpected : Flag<["-"], "verify-ignore-unexpected">,
   HelpText<"Ignore unexpected diagnostic messages">;

clang/lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_ferror_limit_EQ))
     CmdArgs.push_back(A->getValue());
   else
     CmdArgs.push_back("19");
 
   Args.AddLastArg(CmdArgs, options::OPT_fconstexpr_backtrace_limit_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_fmacro_backtrace_limit_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_ftemplate_backtrace_limit_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_fspell_checking_limit_EQ);
+  Args.AddLastArg(CmdArgs, options::OPT_fcaret_diagnostics_max_lines_EQ);
 
   // Pass -fmessage-length=.
   unsigned MessageLength = 0;
   if (Arg *A = Args.getLastArg(options::OPT_fmessage_length_EQ)) {
     StringRef V(A->getValue());
     if (V.getAsInteger(0, MessageLength))
       D.Diag(diag::err_drv_invalid_argument_to_option)
           << V << A->getOption().getName();

clang/test/Driver/caret-diagnostics-max-lines.cpp

+//RUN: %clang++ -### -fcaret-diagnostics-max-lines=2 %s 2>&1 | FileCheck %s
+
+// CHECK: "-fcaret-diagnostics-max-lines=2"

clang/test/FixIt/fixit-function-call.cpp

-// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines 1 -x c++ %s 2> %t
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=1 -x c++ %s 2> %t
 // RUN: FileCheck %s < %t
 // PR5941
 // END.
 
 /* Test fixits for * and & mismatch in function arguments.
  * Since fixits are on the notes, they cannot be applied automatically. */
 
 typedef int intTy;

clang/test/Misc/caret-diags-multiline.cpp

-// RUN: not %clang_cc1 -std=c++11 -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines 5 -Wsometimes-uninitialized %s 2>&1 | FileCheck %s --strict-whitespace
+// RUN: not %clang_cc1 -std=c++11 -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=5 -Wsometimes-uninitialized %s 2>&1 | FileCheck %s --strict-whitespace
 
 void line(int);
 
 // Check we expand the range as much as possible within the limit.
 
 // CHECK:      warning: variable 'a' is used uninitialized whenever 'if' condition is true
 // CHECK-NEXT: {{^}}  if (cond) {
 // CHECK-NEXT: {{^}}      ^~~~{{$}}

clang/test/Sema/caret-diags-complex-init.cpp

-// RUN: not %clang_cc1 -std=c++11 -fsyntax-only -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines 5 %s 2>&1 | FileCheck %s -strict-whitespace
+// RUN: not %clang_cc1 -std=c++11 -fsyntax-only -fno-diagnostics-show-line-numbers -fcaret-diagnostics-max-lines=5 %s 2>&1 | FileCheck %s -strict-whitespace
 
 
 //CHECK: {{.*}}: error: excess elements in scalar initializer
 //CHECK-NEXT: {{^}}_Complex double gz1 = {1, 2, 3};
 //CHECK-NEXT: {{^}}                             ^{{$}}
 _Complex double gz1 = {1, 2, 3}; 
 
 //CHECK: {{.*}}: error: excess elements in scalar initializer