This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tbaeder on Jun 3 2023, 9:34 PM.

Details

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.

Diff Detail

Event Timeline

tbaeder created this revision.Jun 3 2023, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2023, 9:34 PM
tbaeder requested review of this revision.Jun 3 2023, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2023, 9:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dyung added a subscriber: dyung.Jun 5 2023, 1:08 AM
hans added a subscriber: hans.Jun 5 2023, 3:46 AM

Should there be a test?

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 ?

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.

tbaeder edited the summary of this revision. (Show Details)Jun 5 2023, 9:40 PM

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?

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...

tbaeder updated this revision to Diff 529818.Jun 8 2023, 9:30 PM
tbaeder marked an inline comment as done.
MaskRay added inline comments.Jun 8 2023, 9:42 PM
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.

dyung added a comment.Jun 8 2023, 9:58 PM

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 marked an inline comment as done.Jun 8 2023, 9:58 PM
tbaeder added inline comments.
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?

tbaeder updated this revision to Diff 529832.Jun 8 2023, 11:04 PM

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/.

tbaeder updated this revision to Diff 530168.Jun 10 2023, 12:00 AM
tbaeder retitled this revision from [clang][Driver] Add -fcaret-diagnostics-max-lines as a driver option to [clang][Driver] Add -fcaret-diagnostics-max-lines= as a driver option.Jun 10 2023, 12:35 AM
MaskRay accepted this revision.Jun 11 2023, 10:54 AM

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.

This revision is now accepted and ready to land.Jun 11 2023, 10:54 AM
tbaeder marked an inline comment as done.Jun 12 2023, 1:00 AM
tbaeder added inline comments.
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?

This revision was landed with ongoing or failed builds.Jun 12 2023, 3:45 AM
This revision was automatically updated to reflect the committed changes.
tbaeder marked an inline comment as done.

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

Thanks for the fix! 👍