[clang-tidy] New option CompilationArgsToRemoveRegex to remove arguments from the command line
#111453
Conversation
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
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Félix-Antoine Constantin (felix642) ChangesWhen 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. Fixes #108455 Full diff: https://github.com/llvm/llvm-project/pull/111453.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 62f9d19b2a362f..4bd921b819260e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -552,6 +552,30 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
return AdjustedArgs;
};
+ // Remove unwanted arguments passed to the compiler
+ ArgumentsAdjuster CompilationArgsToIgnore =
+ [&Context](const CommandLineArguments &Args, StringRef Filename) {
+ ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
+ CommandLineArguments AdjustedArgs = Args;
+
+ if (Opts.CompilationArgsToRemoveRegex) {
+ for (StringRef ArgToIgnore : *Opts.CompilationArgsToRemoveRegex) {
+ llvm::Regex ArgToIgnoreRegex(ArgToIgnore);
+ AdjustedArgs.erase(
+ std::remove_if(AdjustedArgs.begin(), AdjustedArgs.end(),
+ [&](llvm::StringRef Arg) {
+ return Arg.starts_with("-") &&
+ Arg != "-fsyntax-only" &&
+ ArgToIgnoreRegex.match(Arg);
+ }),
+ AdjustedArgs.end());
+ }
+ }
+
+ return AdjustedArgs;
+ };
+
+ Tool.appendArgumentsAdjuster(CompilationArgsToIgnore);
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 445c7f85c900c6..32112db07b09d2 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -174,6 +174,8 @@ template <> struct MappingTraits<ClangTidyOptions> {
Options.ExcludeHeaderFilterRegex);
IO.mapOptional("FormatStyle", Options.FormatStyle);
IO.mapOptional("User", Options.User);
+ IO.mapOptional("CompilationArgsToRemoveRegex",
+ Options.CompilationArgsToRemoveRegex);
IO.mapOptional("CheckOptions", Options.CheckOptions);
IO.mapOptional("ExtraArgs", Options.ExtraArgs);
IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
@@ -198,6 +200,7 @@ ClangTidyOptions ClangTidyOptions::getDefaults() {
Options.SystemHeaders = false;
Options.FormatStyle = "none";
Options.User = std::nullopt;
+ Options.CompilationArgsToRemoveRegex = std::nullopt;
for (const ClangTidyModuleRegistry::entry &Module :
ClangTidyModuleRegistry::entries())
Options.mergeWith(Module.instantiate()->getModuleOptions(), 0);
@@ -238,6 +241,8 @@ ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
overrideValue(SystemHeaders, Other.SystemHeaders);
overrideValue(FormatStyle, Other.FormatStyle);
overrideValue(User, Other.User);
+ overrideValue(CompilationArgsToRemoveRegex,
+ Other.CompilationArgsToRemoveRegex);
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 85d5a02ebbc1bc..0ccdde1ff3fc31 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -110,6 +110,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>> CompilationArgsToRemoveRegex;
+
/// 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 3e051c7db6adcc..ea3ad5356a897c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,9 @@ Improvements to clang-tidy
- Improved :program:`run-clang-tidy.py` script. Fixed minor shutdown noise
happening on certain platforms when interrupting the script.
+- Improved :program:`clang-tidy` by adding the option `CompilationArgsToRemoveRegex`
+ 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 e38141bdb8be1f..65d96b29079f23 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -303,6 +303,8 @@ 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'.
+ CompilationArgsToRemoveRegex - List of arguments to remove from the command
+ line sent to the compiler.
The effective configuration can be inspected using --dump-config:
@@ -312,6 +314,7 @@ An overview of all the command-line options:
WarningsAsErrors: ''
HeaderFileExtensions: ['', 'h','hh','hpp','hxx']
ImplementationFileExtensions: ['c','cc','cpp','cxx']
+ CompilationArgsToRemoveRegex: ['-Werror', '-f.*']
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 00000000000000..33a42d416e75a7
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/compilation-args-to-ignore.cpp
@@ -0,0 +1,9 @@
+// RUN: not clang-tidy %s -- -fnot-an-option | FileCheck %s -check-prefix=INVALID-A
+// RUN: clang-tidy %s --config="{CompilationArgsToRemoveRegex: ['-fnot-an-option']}" -- -fnot-an-option
+// RUN: not clang-tidy %s --config="{CompilationArgsToRemoveRegex: ['-f.*']}" -- -fnot-an-option -invalid-option | FileCheck %s -check-prefix=INVALID-B
+// RUN: clang-tidy %s --config="{CompilationArgsToRemoveRegex: ['-f.*', '-invalid-option']}" -- -fnot-an-option -fnot-another-option -finvalid-option -invalid-option
+// RUN: not clang-tidy %s --config="{CompilationArgsToRemoveRegex: ['\$invalid-option']}" -- -finvalid-option | FileCheck %s -check-prefix=INVALID-C
+
+// INVALID-A: error: unknown argument: '-fnot-an-option' [clang-diagnostic-error]
+// INVALID-B: error: unknown argument: '-invalid-option' [clang-diagnostic-error]
+// INVALID-C: error: unknown argument: '-finvalid-option' [clang-diagnostic-error]
\ No newline at end of file
|
| - Improved :program:`run-clang-tidy.py` script. Fixed minor shutdown noise | ||
| happening on certain platforms when interrupting the script. | ||
|
|
||
| - Improved :program:`clang-tidy` by adding the option `CompilationArgsToRemoveRegex` |
There was a problem hiding this comment.
Please move it after previous clang-tidy entry. Also ":option:CompilationArgsToRemoveRegex"?
There was a problem hiding this comment.
@EugeneZelenko, I might be wrong, but since the option is only mentioned in index.rst, I don't think I can use :option: in the Release notes.
EugeneZelenko
requested review from
5chmidti,
HerrCai0907,
PiotrZSL and
SimplyDanny
Improved Release Notes
|
This would be a big improvement for users who using another compiler. |
|
Just for discussion:
|
|
I too have the feeling that this feature is a responsibility of the build system, not of clang-tidy. What do you think @AaronBallman ? |
If I understand correctly, this situation is when CMake is configured for another compiler (e.g., GCC) but is also instructed to run clang-tidy, but then CMake hands clang-tidy the command line arguments it was handing to GCC, and clang-tidy is giving hard errors for the unknown command line arguments. Clang itself ignores some unknown command line options while erring on others (it seems to be CC @MaskRay @jansvoboda11 for additional opinions (and why does clang warn in some cases but err in others, is that intentional?) |
Exactly. This option was added to handle this situation similarly to what Clangd already does. They currently have an option Currently, clang-tidy only supports adding arguments through the I think the ideal solution would be to support this feature directly with the build system as highlighted by @carlosgalvezp and maybe add an option to clang-tidy as suggested by @nicovank I have added the argument in clang-tidy since this was fairly simple to support and was already in line with what was done in other tools (Clangd). I don't mind having a more in-depth look into the clang's codebase to see how those unknown arguments could be handled directly by the build system. |
|
The way we do it at our place is that we have 2 toolchains, one for GCC and one for Clang, each with separate sets of flags. We assume that being able to compile with Clang is a prerequisite to running clang-tidy. For example, even if you have only Once that's satisfied, we can run clang-tidy based on the Clang toolchain - this way we ensure all the flags are valid for clang-tidy (since they are for Clang). But I understand if having 2 parallel toolchains can be costly to maintain. Alternatively, you could have a simple wrapper around clang-tidy that filters out the unknown flags, and tell CMake to run that wrapper instead. |
II think a would be great if clang-tidy has an out of the box experience. some project may don't support clang compiler due to CI (e.g. clang and gcc will warning different thing in |
|
I think the regex solution might be brittle/not clean from a user perspective, with potential for a small amount of maintenance. Instead, I would also suggest a flag to error, warn, or silence these, defaulting to a warning. Using
The |
Second this. The option can ignore certain driver errors (unknown argument). |
|
@kwalker-plxs In my opinion, and after reading this PR's comments, a better approach would be to add an option to downgrade the errors clang-tidy currently emits when it doesn't recognize a compilation flag. That way the user could downgrade the errors to warnings if they want to have a feedback on which arguments were ignored or completely ignore them if they don't care. This would be much easier to maintain from a user's perspective since a change to the GCC compilation wouldn't impact the clang-tidy analysis. |
11 participants
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