[clang-tidy] New option to remove arguments from the command line #164344
+38
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Félix-Antoine Constantin (felix642) ChangesWhen using clang-tidy from a compilation database, some options might not be Fixes #108455 Please see #111453 and #162201 for previous discussions regarding this option. The goal is to keep this feature as simple as possible and to make sure there aren't any side effects. So the user has to explicitly list the arguments he wants to remove. A mention was also added to warn the user that removing arguments might lead to false positive / negative if the code isn't properly recognized by the clang compiler. Full diff: https://github.com/llvm/llvm-project/pull/164344.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 7e18f3806a143..6e91e0a254c0f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -581,6 +581,24 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
return AdjustedArgs;
};
+ // Remove unwanted arguments passed to the compiler
+ ArgumentsAdjuster CompilationArgsToRemove =
+ [&Context](const CommandLineArguments &Args, StringRef Filename) {
+ ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
+ CommandLineArguments AdjustedArgs = Args;
+
+ if (Opts.RemovedArgs) {
+ for (StringRef ArgToIgnore : *Opts.RemovedArgs) {
+ AdjustedArgs.erase(std::remove(AdjustedArgs.begin(),
+ AdjustedArgs.end(), ArgToIgnore),
+ AdjustedArgs.end());
+ }
+ }
+
+ return AdjustedArgs;
+ };
+
+ Tool.appendArgumentsAdjuster(CompilationArgsToRemove);
Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);
Tool.appendArgumentsAdjuster(getStripPluginsAdjuster());
Context.setEnableProfiling(EnableCheckProfile);
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index b752a9beb0e34..c82ad1e6e899f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -225,6 +225,7 @@ template <> struct MappingTraits<ClangTidyOptions> {
Options.ExcludeHeaderFilterRegex);
IO.mapOptional("FormatStyle", Options.FormatStyle);
IO.mapOptional("User", Options.User);
+ IO.mapOptional("RemovedArgs", Options.RemovedArgs);
IO.mapOptional("CheckOptions", Options.CheckOptions);
IO.mapOptional("ExtraArgs", Options.ExtraArgs);
IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
@@ -250,6 +251,7 @@ ClangTidyOptions ClangTidyOptions::getDefaults() {
Options.SystemHeaders = false;
Options.FormatStyle = "none";
Options.User = std::nullopt;
+ Options.RemovedArgs = std::nullopt;
for (const ClangTidyModuleRegistry::entry &Module :
ClangTidyModuleRegistry::entries())
Options.mergeWith(Module.instantiate()->getModuleOptions(), 0);
@@ -290,6 +292,7 @@ ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
overrideValue(SystemHeaders, Other.SystemHeaders);
overrideValue(FormatStyle, Other.FormatStyle);
overrideValue(User, Other.User);
+ mergeVectors(RemovedArgs, Other.RemovedArgs);
overrideValue(UseColor, Other.UseColor);
mergeVectors(ExtraArgs, Other.ExtraArgs);
mergeVectors(ExtraArgsBefore, Other.ExtraArgsBefore);
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index 2aae92f1d9eb3..989b1c651c830 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -112,6 +112,10 @@ struct ClangTidyOptions {
/// comments in the relevant check.
std::optional<std::string> User;
+ /// \brief Remove command line arguments sent to the compiler matching this
+ /// regex.
+ std::optional<std::vector<std::string>> RemovedArgs;
+
/// Helper structure for storing option value with priority of the value.
struct ClangTidyValue {
ClangTidyValue() = default;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 62e1987377989..1a6cc0869c571 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -123,6 +123,7 @@ Improvements to clang-tidy
when run over C files. If ``-std`` is not specified, it defaults to
``c99-or-later``.
+<<<<<<< HEAD
- :program:`clang-tidy` no longer attemps to analyze code from system headers
by default, greatly improving performance. This behavior is disabled if the
`SystemHeaders` option is enabled.
@@ -158,6 +159,10 @@ Improvements to clang-tidy
scripts by adding the `-hide-progress` option to suppress progress and
informational messages.
+- Improved :program:`clang-tidy` by adding the option
+ :option:`RemovedArgs` to remove arguments sent to the
+ compiler when invoking clang-tidy.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index bd2c40e948f34..43142c9121716 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -329,6 +329,10 @@ An overview of all the command-line options:
example, to place the correct user name in
TODO() comments in the relevant check.
WarningsAsErrors - Same as '--warnings-as-errors'.
+ RemovedArgs - List of arguments to remove from the command
+ line sent to the compiler. Please note that
+ removing arguments from the command line
+ might lead to false positive or negatives.
The effective configuration can be inspected using --dump-config:
@@ -338,6 +342,7 @@ An overview of all the command-line options:
WarningsAsErrors: ''
HeaderFileExtensions: ['', 'h','hh','hpp','hxx']
ImplementationFileExtensions: ['c','cc','cpp','cxx']
+ RemovedArgs: ['-Werror']
HeaderFilterRegex: ''
FormatStyle: none
InheritParentConfig: true
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/compilation-args-to-ignore.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/compilation-args-to-ignore.cpp
new file mode 100644
index 0000000000000..9a042dedce3a4
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/compilation-args-to-ignore.cpp
@@ -0,0 +1,5 @@
+// RUN: not clang-tidy %s -- -fnot-an-option | FileCheck %s -check-prefix=INVALID-A
+// RUN: clang-tidy %s --config="{RemovedArgs: ['-fnot-an-option']}" -- -fnot-an-option
+// RUN: clang-tidy %s --config="{RemovedArgs: ['-fnot-another-option', '-fnot-an-option']}" -- -fnot-an-option -fnot-another-option
+
+// INVALID-A: error: unknown argument: '-fnot-an-option' [clang-diagnostic-error]
\ No newline at end of file
|
When using clang-tidy from a compilation database, some options might not be recognized by clang if the compilation database was generated for another compiler. This forces the user to add conditional code in their CMakeLists.txt to remove those arguments when using clang-tidy. A new option was added to the .clang-tidy config to remove those unknown flags without the need to generate a second compilation_commands.json Fixes llvm#108455
Improved Release Notes
felix642
force-pushed
the
compiler-cmd-line-args-clang-tidy
branch
from
95cd57b to
608b5f3
Compare
Removed Regex Support
felix642
force-pushed
the
compiler-cmd-line-args-clang-tidy
branch
from
608b5f3 to
f2c15c6
Compare
…mmand line Documentation issues
| WarningsAsErrors: '' | ||
| HeaderFileExtensions: ['', 'h','hh','hpp','hxx'] | ||
| ImplementationFileExtensions: ['c','cc','cpp','cxx'] | ||
| RemovedArgs: ['-Werror'] |
There was a problem hiding this comment.
+1 I think RemoveArgs sounds better
| std::optional<std::string> User; | ||
|
|
||
| /// \brief Remove command line arguments sent to the compiler matching this | ||
| /// regex. |
There was a problem hiding this comment.
It's no longer a regex
| return AdjustedArgs; | ||
| }; | ||
|
|
||
| Tool.appendArgumentsAdjuster(CompilationArgsToRemove); |
There was a problem hiding this comment.
Nit: for consistency with the one for ExtraArgs, maybe we can call this PerFileArgumentRemover?
| CommandLineArguments AdjustedArgs = Args; | ||
|
|
||
| if (Opts.RemovedArgs) { | ||
| for (StringRef ArgToIgnore : *Opts.RemovedArgs) { |
| TODO() comments in the relevant check. | ||
| WarningsAsErrors - Same as '--warnings-as-errors'. | ||
| RemovedArgs - List of arguments to remove from the command | ||
| line sent to the compiler. Please note that |
There was a problem hiding this comment.
Perhaps good to document that this removes the args from the "original" command line, before considering ExtraArgs
| // RUN: clang-tidy %s --config="{RemovedArgs: ['-fnot-an-option']}" -- -fnot-an-option | ||
| // RUN: clang-tidy %s --config="{RemovedArgs: ['-fnot-another-option', '-fnot-an-option']}" -- -fnot-an-option -fnot-another-option | ||
|
|
||
| // INVALID-A: error: unknown argument: '-fnot-an-option' [clang-diagnostic-error] No newline at end of file |
There was a problem hiding this comment.
Missing newline at the end of the file
There was a problem hiding this comment.
Rename to removed-args.cpp for consistency with the option name
| RemovedArgs - List of arguments to remove from the command | ||
| line sent to the compiler. Please note that | ||
| removing arguments from the command line | ||
| might lead to false positive or negatives. |
There was a problem hiding this comment.
What about: might change the semantics of the analyzed code, possibly leading to crashes, false positives or false negatives.
There was a problem hiding this comment.
Warning people about false positives and negatives makes sense, but I don't think we should be telling people to expect crashes; a crash is always a bug, it's not something users should ever expect, right?
There was a problem hiding this comment.
Hmm yes, crash is a strong word. I meant more "hard compiler errors" which cannot simply be suppressed.
| informational messages. | ||
|
|
||
| - Improved :program:`clang-tidy` by adding the option `RemovedArgs` to remove | ||
| arguments sent to the compiler when invoking clang-tidy. |
There was a problem hiding this comment.
Suggested change
| arguments sent to the compiler when invoking clang-tidy. | |
| arguments sent to the compiler when invoking Clang-Tidy. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using clang-tidy from a compilation database, some options might not be
recognized by clang if the compilation database was generated for another compiler.
This forces the user to add conditional code in their CMakeLists.txt to remove
those arguments when using clang-tidy.
A new option was added to the .clang-tidy config to remove
those unknown flags without the need to generate a second compilation_commands.json
Fixes #108455
Please see #111453 and #162201 for previous discussions regarding this option.
This implementation was slightly changed from the initial PR based on the following comment (#162201 (comment)).
The goal is to keep this feature as simple as possible and to make sure there aren't any side effects. So the user has to explicitly list the arguments he wants to remove. A mention was also added to warn the user that removing arguments might lead to false positive / negative if the code isn't properly recognized by the clang compiler.