njames93 created this revision.
njames93 added reviewers: sammccall, kadircet.
Herald added subscribers: usaxena95, arphaman.
njames93 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
Add instrumentation in ConfigCompile to validate that items in
ClangTidy:[Add|Remove] correspond to actual clang-tidy checks.
If they don't a warning will be presented to the user.
This is especially useful for catching typos in the glob items.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D92874
Files:
clang-tools-extra/clang-tidy/ClangTidyModule.h
clang-tools-extra/clang-tidy/GlobList.cpp
clang-tools-extra/clang-tidy/GlobList.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -200,6 +200,28 @@
EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("StrictMode"), "true");
EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("example-check.ExampleOption"),
"0");
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+}
+
+TEST_F(ConfigCompileTests, TidyBadChecks) {
+ Frag.ClangTidy.Add.emplace_back("unknown-check");
+ Frag.ClangTidy.Add.emplace_back("unknown-module-*");
+ Frag.ClangTidy.Remove.emplace_back("*");
+ Frag.ClangTidy.Remove.emplace_back("llvm-includeorder");
+ EXPECT_TRUE(compileAndApply());
+ // Ensure bad checks are stripped from the glob.
+ EXPECT_EQ(Conf.ClangTidy.Checks, "-*");
+ EXPECT_THAT(
+ Diags.Diagnostics,
+ ElementsAre(AllOf(DiagMessage("Check glob 'unknown-check' doesn't "
+ "specify any known clang-tidy check"),
+ DiagKind(llvm::SourceMgr::DK_Warning)),
+ AllOf(DiagMessage("Check glob 'unknown-module-*' doesn't "
+ "specify any known clang-tidy check"),
+ DiagKind(llvm::SourceMgr::DK_Warning)),
+ AllOf(DiagMessage("Check glob 'llvm-includeorder' doesn't "
+ "specify any known clang-tidy check"),
+ DiagKind(llvm::SourceMgr::DK_Warning))));
}
TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -23,6 +23,8 @@
//
//===----------------------------------------------------------------------===//
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "../clang-tidy/GlobList.h"
#include "CompileCommands.h"
#include "Config.h"
#include "ConfigFragment.h"
@@ -30,12 +32,14 @@
#include "Features.inc"
#include "support/Logger.h"
#include "support/Trace.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Allocator.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Format.h"
@@ -347,6 +351,50 @@
}
}
+ // Warn against any Add or Remove check items that doesn't specify a valid
+ // tidy check, most likely due to a typo.
+ bool validateCheckGlob(const Located<std::string> &CheckGlob) {
+ static llvm::ArrayRef<StringRef> AllChecks = [] {
+ // In a simple world this would be a std::vector<std::string>. However,
+ // using the bump allocator and an ArrayRef, we can drastically reduce
+ // number of allocations while simultaneously increasing cache locality.
+ static llvm::BumpPtrAllocator Alloc;
+ tidy::ClangTidyCheckFactories Factories;
+ for (tidy::ClangTidyModuleRegistry::entry E :
+ tidy::ClangTidyModuleRegistry::entries())
+ E.instantiate()->addCheckFactories(Factories);
+ llvm::MutableArrayRef<StringRef> AllChecks(
+ Alloc.Allocate<StringRef>(Factories.size()), Factories.size());
+ llvm::StringRef *Iter = AllChecks.begin();
+ for (const auto &Factory : Factories) {
+ StringRef Key = Factory.getKey();
+ assert(!Key.empty() && "All checks should have valid names");
+ char *Ptr = Alloc.Allocate<char>(Key.size());
+ // Copy the Key into our newly allocated buffer, We don't need to worry
+ // about writing a null terminator.
+ memcpy(Ptr, Key.data(), Key.size());
+ *Iter++ = StringRef(Ptr, Key.size());
+ }
+ assert(Iter == AllChecks.end());
+ return AllChecks;
+ }();
+ tidy::GlobList List(*CheckGlob);
+ // Looping over the list of checks is not great in complexity, but given the
+ // intricacies of glob lists, a set based approach wouldn't really be
+ // feasible.
+ if (llvm::any_of(AllChecks,
+ [&List](const StringRef &S) { return List.specifies(S); }))
+ return true;
+
+ diag(Warning,
+ llvm::formatv(
+ "Check glob '{0}' doesn't specify any known clang-tidy check",
+ *CheckGlob)
+ .str(),
+ CheckGlob.Range);
+ return false;
+ }
+
void appendTidyCheckSpec(std::string &CurSpec,
const Located<std::string> &Arg, bool IsPositive) {
StringRef Str = *Arg;
@@ -364,11 +412,15 @@
void compile(Fragment::ClangTidyBlock &&F) {
std::string Checks;
- for (auto &CheckGlob : F.Add)
- appendTidyCheckSpec(Checks, CheckGlob, true);
+ for (const auto &CheckGlob : F.Add) {
+ if (validateCheckGlob(CheckGlob))
+ appendTidyCheckSpec(Checks, CheckGlob, true);
+ }
- for (auto &CheckGlob : F.Remove)
- appendTidyCheckSpec(Checks, CheckGlob, false);
+ for (const auto &CheckGlob : F.Remove) {
+ if (validateCheckGlob(CheckGlob))
+ appendTidyCheckSpec(Checks, CheckGlob, false);
+ }
if (!Checks.empty())
Out.Apply.push_back(
Index: clang-tools-extra/clang-tidy/GlobList.h
===================================================================
--- clang-tools-extra/clang-tidy/GlobList.h
+++ clang-tools-extra/clang-tidy/GlobList.h
@@ -35,6 +35,10 @@
/// matching glob's Positive flag.
bool contains(StringRef S);
+ /// Returns \c true if the pattern contains an entry matching \p S, Doesn't
+ /// specify if the entry is positive or negative.
+ bool specifies(StringRef S);
+
private:
struct GlobListItem {
Index: clang-tools-extra/clang-tidy/GlobList.cpp
===================================================================
--- clang-tools-extra/clang-tidy/GlobList.cpp
+++ clang-tools-extra/clang-tidy/GlobList.cpp
@@ -61,3 +61,8 @@
}
return false;
}
+
+bool GlobList::specifies(StringRef S) {
+ return llvm::any_of(
+ Items, [&S](const GlobListItem &Item) { return Item.Regex.match(S); });
+}
Index: clang-tools-extra/clang-tidy/ClangTidyModule.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyModule.h
+++ clang-tools-extra/clang-tidy/ClangTidyModule.h
@@ -73,6 +73,7 @@
FactoryMap::const_iterator begin() const { return Factories.begin(); }
FactoryMap::const_iterator end() const { return Factories.end(); }
bool empty() const { return Factories.empty(); }
+ size_t size() const { return Factories.size(); }
private:
FactoryMap Factories;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
