njames93 updated this revision to Diff 310352.
njames93 added a comment.
Made GlobList::specifies const.
Made check validation function static and only taking a StringRef. This will
make it easier to refactor out at a later date if this functionality wants to
be used elsewhere.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92874/new/
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,41 @@
}
}
+ // Warn against any Add or Remove check items that doesn't specify a valid
+ // tidy check, most likely due to a typo.
+ static bool isValidCheckGlob(StringRef 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.
+ return llvm::any_of(
+ AllChecks, [&List](const StringRef &S) { return List.specifies(S); });
+ }
+
void appendTidyCheckSpec(std::string &CurSpec,
const Located<std::string> &Arg, bool IsPositive) {
StringRef Str = *Arg;
@@ -356,6 +395,15 @@
diag(Error, "Invalid clang-tidy check name", Arg.Range);
return;
}
+ if (!isValidCheckGlob(Str)) {
+ diag(Warning,
+ llvm::formatv(
+ "Check glob '{0}' doesn't specify any known clang-tidy check",
+ Str)
+ .str(),
+ Arg.Range);
+ return;
+ }
CurSpec += ',';
if (!IsPositive)
CurSpec += '-';
@@ -364,10 +412,10 @@
void compile(Fragment::ClangTidyBlock &&F) {
std::string Checks;
- for (auto &CheckGlob : F.Add)
+ for (const auto &CheckGlob : F.Add)
appendTidyCheckSpec(Checks, CheckGlob, true);
- for (auto &CheckGlob : F.Remove)
+ for (const auto &CheckGlob : F.Remove)
appendTidyCheckSpec(Checks, CheckGlob, false);
if (!Checks.empty())
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) const;
+ /// 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) const;
+
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) const {
+ 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
