sammccall updated this revision to Diff 478274.
sammccall retitled this revision from "[clangd] Don't run slow clang-tidy
checks" to "[clangd] Don't run slow clang-tidy checks by default".
sammccall edited the summary of this revision.
sammccall added a comment.
Add tests of isFast
Make isFast a tristate (fast/unknown/slow)
Make the config option a tristate filter (strict/loose/none)
Emit diagnostics when enabling a (maybe)slow check in config
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138505/new/
https://reviews.llvm.org/D138505
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/TidyProvider.cpp
clang-tools-extra/clangd/TidyProvider.h
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
Index: clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
@@ -6,8 +6,10 @@
//
//===----------------------------------------------------------------------===//
+#include "Feature.h"
#include "TestFS.h"
#include "TidyProvider.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
#include "gtest/gtest.h"
namespace clang {
@@ -52,6 +54,22 @@
EXPECT_EQ(*Sub2Options.Checks, "misc-*,bugprone-*");
EXPECT_EQ(Sub2Options.CheckOptions.lookup("TestKey").Value, "3");
}
+
+TEST(TidyProvider, IsFastTidyCheck) {
+ EXPECT_THAT(isFastTidyCheck("misc-const-correctness"), llvm::ValueIs(false));
+ EXPECT_THAT(isFastTidyCheck("bugprone-suspicious-include"),
+ llvm::ValueIs(true));
+ // Linked in (ParsedASTTests.cpp) but not measured.
+ EXPECT_EQ(isFastTidyCheck("replay-preamble-check"), llvm::None);
+}
+
+#if CLANGD_TIDY_CHECKS
+TEST(TidyProvider, IsValidCheck) {
+ EXPECT_TRUE(isRegisteredTidyCheck("bugprone-argument-comment"));
+ EXPECT_FALSE(isRegisteredTidyCheck("bugprone-argument-clinic"));
+}
+#endif
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -17,6 +17,7 @@
#include "AST.h"
#include "Annotations.h"
#include "Compiler.h"
+#include "Config.h"
#include "Diagnostics.h"
#include "Headers.h"
#include "ParsedAST.h"
@@ -25,6 +26,7 @@
#include "TestFS.h"
#include "TestTU.h"
#include "TidyProvider.h"
+#include "support/Context.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
@@ -427,6 +429,9 @@
// sense in c++ (also they actually break on windows), just set language to
// obj-c.
TU.ExtraArgs = {"-isystem.", "-xobjective-c"};
+ Config Cfg;
+ Cfg.Diagnostics.ClangTidy.FastCheckFilter = Config::FastCheckPolicy::Loose;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
const auto &AST = TU.build();
const auto &SM = AST.getSourceManager();
Index: clang-tools-extra/clangd/tool/Check.cpp
===================================================================
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -30,6 +30,8 @@
#include "CodeComplete.h"
#include "CompileCommands.h"
#include "Config.h"
+#include "ConfigFragment.h"
+#include "ConfigProvider.h"
#include "Feature.h"
#include "GlobalCompilationDatabase.h"
#include "Hover.h"
@@ -82,15 +84,19 @@
"check-completion",
llvm::cl::desc("Run code-completion at each point (slow)"),
llvm::cl::init(false)};
+llvm::cl::opt<bool> CheckWarnings{
+ "check-warnings",
+ llvm::cl::desc("Print warnings as well as errors"),
+ llvm::cl::init(false)};
// Print (and count) the error-level diagnostics (warnings are ignored).
unsigned showErrors(llvm::ArrayRef<Diag> Diags) {
unsigned ErrCount = 0;
for (const auto &D : Diags) {
- if (D.Severity >= DiagnosticsEngine::Error) {
+ if (D.Severity >= DiagnosticsEngine::Error || CheckWarnings)
elog("[{0}] Line {1}: {2}", D.Name, D.Range.start.line + 1, D.Message);
+ if (D.Severity >= DiagnosticsEngine::Error)
++ErrCount;
- }
}
return ErrCount;
}
@@ -452,8 +458,23 @@
}
log("Testing on source file {0}", File);
+ class OverrideConfigProvider : public config::Provider {
+ std::vector<config::CompiledFragment>
+ getFragments(const config::Params &,
+ config::DiagnosticCallback Diag) const override {
+ config::Fragment F;
+ // If we're timing clang-tidy checks, implicitly disabling the slow ones
+ // is counterproductive!
+ if (CheckTidyTime.getNumOccurrences())
+ F.Diagnostics.ClangTidy.FastCheckFilter.emplace("None");
+ return {std::move(F).compile(Diag)};
+ }
+ } OverrideConfig;
+ auto ConfigProvider =
+ config::Provider::combine({Opts.ConfigProvider, &OverrideConfig});
+
auto ContextProvider = ClangdServer::createConfiguredContextProvider(
- Opts.ConfigProvider, nullptr);
+ ConfigProvider.get(), nullptr);
WithContext Ctx(ContextProvider(
FakeFile.empty()
? File
Index: clang-tools-extra/clangd/TidyProvider.h
===================================================================
--- clang-tools-extra/clangd/TidyProvider.h
+++ clang-tools-extra/clangd/TidyProvider.h
@@ -60,6 +60,10 @@
/// \pre \p must not be empty, must not contain '*' or ',' or start with '-'.
bool isRegisteredTidyCheck(llvm::StringRef Check);
+/// Returns if \p Check is known-fast, known-slow, or its speed is unknown.
+/// By default, only fast checks will run in clangd.
+llvm::Optional<bool> isFastTidyCheck(llvm::StringRef Check);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/TidyProvider.cpp
===================================================================
--- clang-tools-extra/clangd/TidyProvider.cpp
+++ clang-tools-extra/clangd/TidyProvider.cpp
@@ -213,13 +213,7 @@
// tries to build an AST.
"-bugprone-use-after-move",
// Alias for bugprone-use-after-move.
- "-hicpp-invalid-access-moved",
-
- // ----- Performance problems -----
-
- // This check runs expensive analysis for each variable.
- // It has been observed to increase reparse time by 10x.
- "-misc-const-correctness");
+ "-hicpp-invalid-access-moved");
size_t Size = BadChecks.size();
for (const std::string &Str : ExtraBadChecks) {
@@ -308,5 +302,18 @@
return AllChecks.contains(Check);
}
+
+llvm::Optional<bool> isFastTidyCheck(llvm::StringRef Check) {
+ static auto &Fast = *new llvm::StringMap<bool>{
+#define FAST(CHECK, TIME) {#CHECK,true},
+#define SLOW(CHECK, TIME) {#CHECK,false},
+#include "TidyFastChecks.inc"
+ };
+ auto It = Fast.find(Check);
+ if (It == Fast.end())
+ return llvm::None;
+ return It->second;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -345,6 +345,7 @@
std::shared_ptr<const PreambleData> Preamble) {
trace::Span Tracer("BuildAST");
SPAN_ATTACH(Tracer, "File", Filename);
+ const Config &Cfg = Config::current();
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
if (Preamble && Preamble->StatCache)
@@ -476,6 +477,18 @@
tidy::ClangTidyCheckFactories CTFactories;
for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
E.instantiate()->addCheckFactories(CTFactories);
+ if (Cfg.Diagnostics.ClangTidy.FastCheckFilter !=
+ Config::FastCheckPolicy::None) {
+ // Can't remove checks from a factories container, so create a new one.
+ bool AllowUnknown = Cfg.Diagnostics.ClangTidy.FastCheckFilter ==
+ Config::FastCheckPolicy::Loose;
+ tidy::ClangTidyCheckFactories FastFactories;
+ for (const auto &Factory : CTFactories) {
+ if (isFastTidyCheck(Factory.getKey()).value_or(AllowUnknown))
+ FastFactories.registerCheckFactory(Factory.first(), Factory.second);
+ }
+ CTFactories = std::move(FastFactories);
+ }
CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
@@ -504,7 +517,6 @@
SourceLocation());
}
- const Config &Cfg = Config::current();
ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
if (Cfg.Diagnostics.SuppressAll ||
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -152,6 +152,10 @@
});
CheckOptDict.parse(N);
});
+ Dict.handle("FastCheckFilter", [&](Node &N) {
+ if (auto FastCheckFilter = scalarValue(N, "FastCheckFilter"))
+ F.FastCheckFilter = *FastCheckFilter;
+ });
Dict.parse(N);
}
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -264,6 +264,13 @@
/// readability-braces-around-statements.ShortStatementLines: 2
std::vector<std::pair<Located<std::string>, Located<std::string>>>
CheckOptions;
+
+ /// Whether to run checks that may slow down clangd.
+ /// Strict: Run only checks measured to be fast. (Default)
+ /// This excludes recently-added checks we have not timed yet.
+ /// Loose: Run checks unless they are known to be slow.
+ /// None: Run checks regardless of their speed.
+ llvm::Optional<Located<std::string>> FastCheckFilter;
};
ClangTidyBlock ClangTidy;
};
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -323,11 +323,11 @@
void compile(Fragment::IndexBlock &&F) {
if (F.Background) {
- if (auto Val = compileEnum<Config::BackgroundPolicy>("Background",
- **F.Background)
- .map("Build", Config::BackgroundPolicy::Build)
- .map("Skip", Config::BackgroundPolicy::Skip)
- .value())
+ if (auto Val =
+ compileEnum<Config::BackgroundPolicy>("Background", *F.Background)
+ .map("Build", Config::BackgroundPolicy::Build)
+ .map("Skip", Config::BackgroundPolicy::Skip)
+ .value())
Out.Apply.push_back(
[Val](const Params &, Config &C) { C.Index.Background = *Val; });
}
@@ -433,7 +433,7 @@
if (F.UnusedIncludes)
if (auto Val = compileEnum<Config::UnusedIncludesPolicy>(
- "UnusedIncludes", **F.UnusedIncludes)
+ "UnusedIncludes", *F.UnusedIncludes)
.map("Strict", Config::UnusedIncludesPolicy::Strict)
.map("None", Config::UnusedIncludesPolicy::None)
.value())
@@ -474,11 +474,31 @@
diag(Error, "Invalid clang-tidy check name", Arg.Range);
return;
}
- if (!Str.contains('*') && !isRegisteredTidyCheck(Str)) {
- diag(Warning,
- llvm::formatv("clang-tidy check '{0}' was not found", Str).str(),
- Arg.Range);
- return;
+ if (!Str.contains('*')) {
+ if (!isRegisteredTidyCheck(Str)) {
+ diag(Warning,
+ llvm::formatv("clang-tidy check '{0}' was not found", Str).str(),
+ Arg.Range);
+ return;
+ }
+ auto Fast = isFastTidyCheck(Str);
+ if (!Fast.has_value()) {
+ diag(Warning,
+ llvm::formatv(
+ "Speed of clang-tidy check '{0}' is not known. "
+ "It will only run if ClangTidy.FastCheckFilter is Loose or None",
+ Str)
+ .str(),
+ Arg.Range);
+ } else if (!*Fast) {
+ diag(Warning,
+ llvm::formatv(
+ "clang-tidy check '{0}' is slow. "
+ "It will only run if ClangTidy.FastCheckFilter is None",
+ Str)
+ .str(),
+ Arg.Range);
+ }
}
CurSpec += ',';
if (!IsPositive)
@@ -514,6 +534,16 @@
StringPair.first, StringPair.second);
});
}
+ if (F.FastCheckFilter.has_value())
+ if (auto Val = compileEnum<Config::FastCheckPolicy>("FastCheckFilter",
+ *F.FastCheckFilter)
+ .map("Strict", Config::FastCheckPolicy::Strict)
+ .map("Loose", Config::FastCheckPolicy::Loose)
+ .map("None", Config::FastCheckPolicy::None)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.Diagnostics.ClangTidy.FastCheckFilter = *Val;
+ });
}
void compile(Fragment::DiagnosticsBlock::IncludesBlock &&F) {
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -89,6 +89,7 @@
} Index;
enum UnusedIncludesPolicy { Strict, None };
+ enum class FastCheckPolicy { Strict, Loose, None };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -99,6 +100,7 @@
// A comma-seperated list of globs specify which clang-tidy checks to run.
std::string Checks;
llvm::StringMap<std::string> CheckOptions;
+ FastCheckPolicy FastCheckFilter = FastCheckPolicy::Strict;
} ClangTidy;
UnusedIncludesPolicy UnusedIncludes = None;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
