[clang-tools-extra] [clang-tidy] Improved --verify-config when using literal style in config file (PR #85591)
Félix-Antoine Constantin via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 26 09:25:10 PDT 2024
https://github.com/felix642 updated https://github.com/llvm/llvm-project/pull/85591
>From f015496c511b4c2898b9f4d65ebfe39a34c5119c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix-Antoine=20Constantin?=
<felix-antoine.constantin at polymtl.ca>
Date: Sun, 17 Mar 2024 20:50:17 -0400
Subject: [PATCH 1/4] [clang-tidy] Improved --verify-config when using literal
style in config file
Specifying checks using the literal style (|) in the clang-tidy config file is currently
supported but was not implemented for the --verify-config options.
This means that clang-tidy would work properly but, using the --verify-config option
would raise an error due to some checks not being parsed properly.
Fixes #53737
---
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp | 9 ++++++---
clang-tools-extra/docs/ReleaseNotes.rst | 3 +++
.../test/clang-tidy/infrastructure/verify-config.cpp | 12 ++++++++++++
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index 9f3d6b6db6cbca..b68a6215b383a6 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -454,11 +454,14 @@ static constexpr StringLiteral VerifyConfigWarningEnd = " [-verify-config]\n";
static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
StringRef Source) {
- llvm::StringRef Cur, Rest;
+ llvm::StringRef Cur = CheckGlob;
+ llvm::StringRef Rest = CheckGlob;
bool AnyInvalid = false;
- for (std::tie(Cur, Rest) = CheckGlob.split(',');
- !(Cur.empty() && Rest.empty()); std::tie(Cur, Rest) = Rest.split(',')) {
+ while (!Cur.empty() || !Rest.empty()) {
+ Cur = Rest.substr(0, Rest.find_first_of(",\n"));
+ Rest = Rest.substr(Cur.size() + 1);
Cur = Cur.trim();
+
if (Cur.empty())
continue;
Cur.consume_front("-");
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a604e9276668ae..3887384e713896 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -262,6 +262,9 @@ Miscellaneous
option is specified. Now ``clang-apply-replacements`` applies formatting only with
the option.
+- Fixed ``--verify-check`` option not properly parsing checks when using the
+ literal operator in the ``.clang-tidy`` config
+
Improvements to include-fixer
-----------------------------
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp
index 421f8641281acb..3659285986482a 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp
@@ -18,3 +18,15 @@
// CHECK-VERIFY: command-line option '-checks': warning: check glob 'bad*glob' doesn't match any known check [-verify-config]
// CHECK-VERIFY: command-line option '-checks': warning: unknown check 'llvm-includeorder'; did you mean 'llvm-include-order' [-verify-config]
// CHECK-VERIFY: command-line option '-checks': warning: unknown check 'my-made-up-check' [-verify-config]
+
+// RUN: echo -e 'Checks: |\n bugprone-argument-comment\n bugprone-assert-side-effect,\n bugprone-bool-pointer-implicit-conversion\n readability-use-anyof*' > %T/MyClangTidyConfig
+// RUN: clang-tidy -verify-config \
+// RUN: --config-file=%T/MyClangTidyConfig | FileCheck %s -check-prefix=CHECK-VERIFY-BLOCK-OK
+// CHECK-VERIFY-BLOCK-OK: No config errors detected.
+
+// RUN: echo -e 'Checks: |\n bugprone-arguments-*\n bugprone-assert-side-effects\n bugprone-bool-pointer-implicit-conversion' > %T/MyClangTidyConfigBad
+// RUN: not clang-tidy -verify-config \
+// RUN: --config-file=%T/MyClangTidyConfigBad 2>&1 | FileCheck %s -check-prefix=CHECK-VERIFY-BLOCK-BAD
+// CHECK-VERIFY-BLOCK-BAD: command-line option '-config': warning: check glob 'bugprone-arguments-*' doesn't match any known check [-verify-config]
+// CHECK-VERIFY-BLOCK-BAD: command-line option '-config': warning: unknown check 'bugprone-assert-side-effects'; did you mean 'bugprone-assert-side-effect' [-verify-config]
+
>From 0e2cb54934829f42069b06f599fa9e724bdf40b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix-Antoine=20Constantin?=
<felix-antoine.constantin at polymtl.ca>
Date: Sat, 23 Mar 2024 13:30:28 -0400
Subject: [PATCH 2/4] fixup! [clang-tidy] Improved --verify-config when using
literal style in config file
Partially rewrote verifyChecks() to realign the checks parsing logic.
Instead of manually parsing the string of globs, verifyCHecks() now uses a GlobList which is responsible
to parse the string and return a list of regexes. This ensures that any changes made to the checks parsing logic
has to be made at only one place rather than two.
---
clang-tools-extra/clang-tidy/GlobList.cpp | 14 ++--
clang-tools-extra/clang-tidy/GlobList.h | 4 ++
.../clang-tidy/tool/ClangTidyMain.cpp | 64 ++++++-------------
3 files changed, 34 insertions(+), 48 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/GlobList.cpp b/clang-tools-extra/clang-tidy/GlobList.cpp
index dfe3f7c505b174..7b8abcdafd2938 100644
--- a/clang-tools-extra/clang-tidy/GlobList.cpp
+++ b/clang-tools-extra/clang-tidy/GlobList.cpp
@@ -19,12 +19,17 @@ static bool consumeNegativeIndicator(StringRef &GlobList) {
return GlobList.consume_front("-");
}
-// Converts first glob from the comma-separated list of globs to Regex and
-// removes it and the trailing comma from the GlobList.
-static llvm::Regex consumeGlob(StringRef &GlobList) {
+// Extract the first glob from the comma-separated list of globs
+// removes it and the trailing comma from the GlobList and
+// returns the extracted glob.
+static llvm::StringRef extractNextGlob(StringRef &GlobList) {
StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find_first_of(",\n"));
StringRef Glob = UntrimmedGlob.trim();
GlobList = GlobList.substr(UntrimmedGlob.size() + 1);
+ return Glob;
+}
+
+static llvm::Regex createRegexFromGlob(StringRef &Glob) {
SmallString<128> RegexText("^");
StringRef MetaChars("()^$|*+?.[]\\{}");
for (char C : Glob) {
@@ -43,7 +48,8 @@ GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
do {
GlobListItem Item;
Item.IsPositive = !consumeNegativeIndicator(Globs);
- Item.Regex = consumeGlob(Globs);
+ Item.Text = extractNextGlob(Globs);
+ Item.Regex = createRegexFromGlob(Item.Text);
if (Item.IsPositive || KeepNegativeGlobs)
Items.push_back(std::move(Item));
} while (!Globs.empty());
diff --git a/clang-tools-extra/clang-tidy/GlobList.h b/clang-tools-extra/clang-tidy/GlobList.h
index 44af182e43b002..4317928270adff 100644
--- a/clang-tools-extra/clang-tidy/GlobList.h
+++ b/clang-tools-extra/clang-tidy/GlobList.h
@@ -44,8 +44,12 @@ class GlobList {
struct GlobListItem {
bool IsPositive;
llvm::Regex Regex;
+ llvm::StringRef Text;
};
SmallVector<GlobListItem, 0> Items;
+
+public:
+ const SmallVectorImpl<GlobListItem> &getItems() const { return Items; };
};
/// A \p GlobList that caches search results, so that search is performed only
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index b68a6215b383a6..16f37c073682cf 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -454,55 +454,31 @@ static constexpr StringLiteral VerifyConfigWarningEnd = " [-verify-config]\n";
static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
StringRef Source) {
- llvm::StringRef Cur = CheckGlob;
- llvm::StringRef Rest = CheckGlob;
+ GlobList Globs(CheckGlob);
bool AnyInvalid = false;
- while (!Cur.empty() || !Rest.empty()) {
- Cur = Rest.substr(0, Rest.find_first_of(",\n"));
- Rest = Rest.substr(Cur.size() + 1);
- Cur = Cur.trim();
-
- if (Cur.empty())
- continue;
- Cur.consume_front("-");
- if (Cur.starts_with("clang-diagnostic"))
+ for (const auto &Item : Globs.getItems()) {
+ const llvm::Regex &Reg = Item.Regex;
+ const llvm::StringRef Text = Item.Text;
+ if (Text.starts_with("clang-diagnostic"))
continue;
- if (Cur.contains('*')) {
- SmallString<128> RegexText("^");
- StringRef MetaChars("()^$|*+?.[]\\{}");
- for (char C : Cur) {
- if (C == '*')
- RegexText.push_back('.');
- else if (MetaChars.contains(C))
- RegexText.push_back('\\');
- RegexText.push_back(C);
- }
- RegexText.push_back('$');
- llvm::Regex Glob(RegexText);
- std::string Error;
- if (!Glob.isValid(Error)) {
- AnyInvalid = true;
- llvm::WithColor::error(llvm::errs(), Source)
- << "building check glob '" << Cur << "' " << Error << "'\n";
- continue;
- }
- if (llvm::none_of(AllChecks.keys(),
- [&Glob](StringRef S) { return Glob.match(S); })) {
- AnyInvalid = true;
+ if (llvm::none_of(AllChecks.keys(), [&Reg](StringRef S) {
+ llvm::errs() << S << '\n';
+ return Reg.match(S);
+ })) {
+ AnyInvalid = true;
+ if (Item.Text.contains('*'))
llvm::WithColor::warning(llvm::errs(), Source)
- << "check glob '" << Cur << "' doesn't match any known check"
+ << "check glob '" << Text << "' doesn't match any known check"
<< VerifyConfigWarningEnd;
+ else {
+ llvm::raw_ostream &Output =
+ llvm::WithColor::warning(llvm::errs(), Source)
+ << "unknown check '" << Text << '\'';
+ llvm::StringRef Closest = closest(Text, AllChecks);
+ if (!Closest.empty())
+ Output << "; did you mean '" << Closest << '\'';
+ Output << VerifyConfigWarningEnd;
}
- } else {
- if (AllChecks.contains(Cur))
- continue;
- AnyInvalid = true;
- llvm::raw_ostream &Output = llvm::WithColor::warning(llvm::errs(), Source)
- << "unknown check '" << Cur << '\'';
- llvm::StringRef Closest = closest(Cur, AllChecks);
- if (!Closest.empty())
- Output << "; did you mean '" << Closest << '\'';
- Output << VerifyConfigWarningEnd;
}
}
return AnyInvalid;
>From 7fe3a903e705dc310948be30d16ce95be1519e6a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix-Antoine=20Constantin?=
<felix-antoine.constantin at polymtl.ca>
Date: Sat, 23 Mar 2024 13:31:25 -0400
Subject: [PATCH 3/4] fixup! [clang-tidy] Improved --verify-config when using
literal style in config file
Fixed small type in release notes
---
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3887384e713896..6b2a2d0cdcf12a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -263,7 +263,7 @@ Miscellaneous
the option.
- Fixed ``--verify-check`` option not properly parsing checks when using the
- literal operator in the ``.clang-tidy`` config
+ literal operator in the ``.clang-tidy`` config.
Improvements to include-fixer
-----------------------------
>From 5a713e7858cc360dfdd292eab5dc54a6c15c14e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix-Antoine=20Constantin?=
<felix-antoine.constantin at bidgroup.ca>
Date: Tue, 26 Mar 2024 12:24:47 -0400
Subject: [PATCH 4/4] fixup! [clang-tidy] Improved --verify-config when using
literal style in config file
Fixed minor issues found in code review
---
clang-tools-extra/clang-tidy/GlobList.cpp | 2 +-
.../clang-tidy/tool/ClangTidyMain.cpp | 15 ++++++---------
clang-tools-extra/docs/ReleaseNotes.rst | 6 +++---
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/GlobList.cpp b/clang-tools-extra/clang-tidy/GlobList.cpp
index 7b8abcdafd2938..8f09ee075bbd6e 100644
--- a/clang-tools-extra/clang-tidy/GlobList.cpp
+++ b/clang-tools-extra/clang-tidy/GlobList.cpp
@@ -19,7 +19,7 @@ static bool consumeNegativeIndicator(StringRef &GlobList) {
return GlobList.consume_front("-");
}
-// Extract the first glob from the comma-separated list of globs
+// Extracts the first glob from the comma-separated list of globs,
// removes it and the trailing comma from the GlobList and
// returns the extracted glob.
static llvm::StringRef extractNextGlob(StringRef &GlobList) {
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index 16f37c073682cf..d7a9e2ab7ea6f9 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -457,24 +457,21 @@ static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
GlobList Globs(CheckGlob);
bool AnyInvalid = false;
for (const auto &Item : Globs.getItems()) {
- const llvm::Regex &Reg = Item.Regex;
- const llvm::StringRef Text = Item.Text;
- if (Text.starts_with("clang-diagnostic"))
+ if (Item.Text.starts_with("clang-diagnostic"))
continue;
- if (llvm::none_of(AllChecks.keys(), [&Reg](StringRef S) {
- llvm::errs() << S << '\n';
- return Reg.match(S);
+ if (llvm::none_of(AllChecks.keys(), [&Item](StringRef S) {
+ return Item.Regex.match(S);
})) {
AnyInvalid = true;
if (Item.Text.contains('*'))
llvm::WithColor::warning(llvm::errs(), Source)
- << "check glob '" << Text << "' doesn't match any known check"
+ << "check glob '" << Item.Text << "' doesn't match any known check"
<< VerifyConfigWarningEnd;
else {
llvm::raw_ostream &Output =
llvm::WithColor::warning(llvm::errs(), Source)
- << "unknown check '" << Text << '\'';
- llvm::StringRef Closest = closest(Text, AllChecks);
+ << "unknown check '" << Item.Text << '\'';
+ llvm::StringRef Closest = closest(Item.Text, AllChecks);
if (!Closest.empty())
Output << "; did you mean '" << Closest << '\'';
Output << VerifyConfigWarningEnd;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b2a2d0cdcf12a..21d6846fad518e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -101,6 +101,9 @@ Improvements to clang-tidy
to filter source files from the compilation database, via a RegEx. In a
similar fashion to what `-header-filter` does for header files.
+- Fixed ``--verify-config`` option not properly parsing checks when using the
+ literal operator in the ``.clang-tidy`` config.
+
New checks
^^^^^^^^^^
@@ -262,9 +265,6 @@ Miscellaneous
option is specified. Now ``clang-apply-replacements`` applies formatting only with
the option.
-- Fixed ``--verify-check`` option not properly parsing checks when using the
- literal operator in the ``.clang-tidy`` config.
-
Improvements to include-fixer
-----------------------------
More information about the cfe-commits
mailing list