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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
+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? |
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...
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 |
Add -fcaret-diagnostics-max-lines as a driver option
-fcaret-diagnostics-max-lines= is more accurate.
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?
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? |
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/. |
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. |
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? |
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
This should have HelpText as well, right?