sammccall updated this revision to Diff 173357.
sammccall marked an inline comment as done.
sammccall added a comment.
Address comments and rebase on https://reviews.llvm.org/D54309, addressing
performance issues.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54204
Files:
clang-tidy/misc/UnusedParametersCheck.cpp
clang-tidy/modernize/LoopConvertCheck.cpp
clang-tidy/modernize/LoopConvertUtils.h
clang-tidy/readability/SimplifyBooleanExprCheck.cpp
clang-tidy/readability/SimplifyBooleanExprCheck.h
clangd/CMakeLists.txt
clangd/ClangdUnit.cpp
clangd/XRefs.cpp
unittests/clangd/ClangdUnitTests.cpp
Index: unittests/clangd/ClangdUnitTests.cpp
===================================================================
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -24,6 +24,7 @@
using testing::Field;
using testing::IsEmpty;
using testing::Pair;
+using testing::UnorderedElementsAre;
testing::Matcher<const Diag &> WithFix(testing::Matcher<Fix> FixMatcher) {
return Field(&Diag::Fixes, ElementsAre(FixMatcher));
@@ -128,6 +129,30 @@
WithFix(Fix(Test.range(), "int", "change return type to 'int'")))));
}
+TEST(DiagnosticsTest, ClangTidy) {
+ Annotations Test(R"cpp(
+ #define $macrodef[[SQUARE]](X) (X)*(X)
+ int main() {
+ return [[sizeof]](sizeof(int));
+ int y = 4;
+ return SQUARE($macroarg[[++]]y);
+ }
+ )cpp");
+ auto TU = TestTU::withCode(Test.code());
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Test.range(), "suspicious usage of 'sizeof(sizeof(...))' "
+ "[bugprone-sizeof-expression]"),
+ AllOf(
+ Diag(Test.range("macroarg"),
+ "side effects in the 1st macro argument 'X' are repeated in "
+ "macro expansion [bugprone-macro-repeated-side-effects]"),
+ WithNote(Diag(Test.range("macrodef"),
+ "macro 'SQUARE' defined here "
+ "[bugprone-macro-repeated-side-effects]")))));
+}
+
TEST(DiagnosticsTest, Preprocessor) {
// This looks like a preamble, but there's an #else in the middle!
// Check that:
Index: clangd/XRefs.cpp
===================================================================
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -672,8 +672,7 @@
return {};
DeducedTypeVisitor V(SourceLocationBeg);
- for (Decl *D : AST.getLocalTopLevelDecls())
- V.TraverseDecl(D);
+ V.TraverseAST(AST.getASTContext());
return V.getDeducedType();
}
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -8,6 +8,8 @@
//===----------------------------------------------------------------------===//
#include "ClangdUnit.h"
+#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
#include "Compiler.h"
#include "Diagnostics.h"
#include "Logger.h"
@@ -151,6 +153,38 @@
return None;
}
+ // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists.
+ // Clang-tidy has some limitiations to ensure reasonable performance:
+ // - checks don't see all preprocessor events in the preamble
+ // - matchers run only over the main-file top-level decls (and can't see
+ // ancestors outside this scope).
+ // In practice almost all checks work well without modifications.
+ std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
+ ast_matchers::MatchFinder CTFinder;
+ llvm::Optional<tidy::ClangTidyContext> CTContext;
+ {
+ trace::Span Tracer("ClangTidyInit");
+ tidy::ClangTidyCheckFactories CTFactories;
+ for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
+ E.instantiate()->addCheckFactories(CTFactories);
+ auto CTOpts = tidy::ClangTidyOptions::getDefaults();
+ // FIXME: this needs to be configurable, and we need to support .clang-tidy
+ // files and other options providers.
+ // These checks exercise the matcher- and preprocessor-based hooks.
+ CTOpts.Checks =
+ "bugprone-sizeof-expression,bugprone-macro-repeated-side-effects";
+ CTContext.emplace(llvm::make_unique<tidy::DefaultOptionsProvider>(
+ tidy::ClangTidyGlobalOptions(), CTOpts));
+ CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
+ CTContext->setASTContext(&Clang->getASTContext());
+ CTContext->setCurrentFile(MainInput.getFile());
+ CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ for (const auto &Check : CTChecks) {
+ Check->registerPPCallbacks(*Clang);
+ Check->registerMatchers(&CTFinder);
+ }
+ }
+
// Copy over the includes from the preamble, then combine with the
// non-preamble includes below.
auto Includes = Preamble ? Preamble->Includes : IncludeStructure{};
@@ -160,13 +194,26 @@
if (!Action->Execute())
log("Execute() failed when building AST for {0}", MainInput.getFile());
+ std::vector<Decl *> ParsedDecls = Action->takeTopLevelDecls();
+ // AST traversals should exclude the preamble, to avoid performance cliffs.
+ Clang->getASTContext().setTraversalScope(ParsedDecls);
+ {
+ // Run the AST-dependent part of the clang-tidy checks.
+ // (The preprocessor part ran already, via PPCallbacks).
+ trace::Span Tracer("ClangTidyMatch");
+ CTFinder.matchAST(Clang->getASTContext());
+ }
+
// UnitDiagsConsumer is local, we can not store it in CompilerInstance that
// has a longer lifetime.
Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
// CompilerInstance won't run this callback, do it directly.
ASTDiags.EndSourceFile();
+ // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
+ // However Action->EndSourceFile() would destroy the ASTContext!
+ // So just inform the preprocessor of EOF, while keeping everything alive.
+ Clang->getPreprocessor().EndSourceFile();
- std::vector<Decl *> ParsedDecls = Action->takeTopLevelDecls();
std::vector<Diag> Diags = ASTDiags.take();
// Add diagnostics from the preamble, if any.
if (Preamble)
@@ -182,7 +229,12 @@
ParsedAST::~ParsedAST() {
if (Action) {
- Action->EndSourceFile();
+ // We already notified the PP of end-of-file earlier, so detach it first.
+ // We must keep it alive until after EndSourceFile(), Sema relies on this.
+ auto PP = Clang->getPreprocessorPtr(); // Keep PP alive for now.
+ Clang->setPreprocessor(nullptr); // Detach so we don't send EOF again.
+ Action->EndSourceFile(); // Destroy ASTContext and Sema.
+ // Now Sema is gone, it's safe for PP to go out of scope.
}
}
@@ -416,4 +468,29 @@
}
} // namespace clangd
+namespace tidy {
+// Force the linker to link in Clang-tidy modules.
+#define LINK_TIDY_MODULE(X) \
+ extern volatile int X##ModuleAnchorSource; \
+ static int LLVM_ATTRIBUTE_UNUSED X##ModuleAnchorDestination = \
+ X##ModuleAnchorSource
+LINK_TIDY_MODULE(CERT);
+LINK_TIDY_MODULE(Abseil);
+LINK_TIDY_MODULE(Boost);
+LINK_TIDY_MODULE(Bugprone);
+LINK_TIDY_MODULE(LLVM);
+LINK_TIDY_MODULE(CppCoreGuidelines);
+LINK_TIDY_MODULE(Fuchsia);
+LINK_TIDY_MODULE(Google);
+LINK_TIDY_MODULE(Android);
+LINK_TIDY_MODULE(Misc);
+LINK_TIDY_MODULE(Modernize);
+LINK_TIDY_MODULE(Performance);
+LINK_TIDY_MODULE(Portability);
+LINK_TIDY_MODULE(Readability);
+LINK_TIDY_MODULE(ObjC);
+LINK_TIDY_MODULE(HICPP);
+LINK_TIDY_MODULE(Zircon);
+#undef LINK_TIDY_MODULE
+} // namespace tidy
} // namespace clang
Index: clangd/CMakeLists.txt
===================================================================
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -64,6 +64,24 @@
clangLex
clangSema
clangSerialization
+ clangTidy
+ clangTidyAndroidModule
+ clangTidyAbseilModule
+ clangTidyBoostModule
+ clangTidyBugproneModule
+ clangTidyCERTModule
+ clangTidyCppCoreGuidelinesModule
+ clangTidyFuchsiaModule
+ clangTidyGoogleModule
+ clangTidyHICPPModule
+ clangTidyLLVMModule
+ clangTidyMiscModule
+ clangTidyModernizeModule
+ clangTidyObjCModule
+ clangTidyPerformanceModule
+ clangTidyPortabilityModule
+ clangTidyReadabilityModule
+ clangTidyZirconModule
clangTooling
clangToolingCore
clangToolingInclusions
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -79,6 +79,7 @@
SourceLocation Loc, StringRef Description,
SourceRange ReplacementRange, StringRef Replacement);
+ bool MatchedOnce = false;
const bool ChainedConditionalReturn;
const bool ChainedConditionalAssignment;
};
Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp
===================================================================
--- clang-tidy/readability/SimplifyBooleanExprCheck.cpp
+++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp
@@ -507,8 +507,15 @@
ChainedConditionalAssignment);
}
+// This is a silly hack to let us run a RecursiveASTVisitor on the Context.
+AST_MATCHER_P(Decl, matchOnce, bool *, Matched) {
+ if (*Matched)
+ return false;
+ return *Matched = true;
+}
+
void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(translationUnitDecl().bind("top"), this);
+ Finder->addMatcher(matchOnce(&MatchedOnce), this);
matchBoolCondition(Finder, true, ConditionThenStmtId);
matchBoolCondition(Finder, false, ConditionElseStmtId);
@@ -556,8 +563,8 @@
else if (const auto *Compound =
Result.Nodes.getNodeAs<CompoundStmt>(CompoundNotBoolId))
replaceCompoundReturnWithCondition(Result, Compound, true);
- else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
- Visitor(this, Result).TraverseDecl(const_cast<Decl*>(TU));
+ else // MatchOnce matcher
+ Visitor(this, Result).TraverseAST(*Result.Context);
}
void SimplifyBooleanExprCheck::issueDiag(
Index: clang-tidy/modernize/LoopConvertUtils.h
===================================================================
--- clang-tidy/modernize/LoopConvertUtils.h
+++ clang-tidy/modernize/LoopConvertUtils.h
@@ -59,9 +59,9 @@
/// \brief Run the analysis on the TranslationUnitDecl.
///
/// In case we're running this analysis multiple times, don't repeat the work.
- void gatherAncestors(const clang::TranslationUnitDecl *T) {
+ void gatherAncestors(ASTContext &Ctx) {
if (StmtAncestors.empty())
- TraverseDecl(const_cast<clang::TranslationUnitDecl *>(T));
+ TraverseAST(Ctx);
}
/// Accessor for StmtAncestors.
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -899,7 +899,7 @@
// variable declared inside the loop outside of it.
// FIXME: Determine when the external dependency isn't an expression converted
// by another loop.
- TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl());
+ TUInfo->getParentFinder().gatherAncestors(*Context);
DependencyFinderASTVisitor DependencyFinder(
&TUInfo->getParentFinder().getStmtToParentStmtMap(),
&TUInfo->getParentFinder().getDeclToParentStmtMap(),
Index: clang-tidy/misc/UnusedParametersCheck.cpp
===================================================================
--- clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tidy/misc/UnusedParametersCheck.cpp
@@ -74,7 +74,7 @@
class UnusedParametersCheck::IndexerVisitor
: public RecursiveASTVisitor<IndexerVisitor> {
public:
- IndexerVisitor(TranslationUnitDecl *Top) { TraverseDecl(Top); }
+ IndexerVisitor(ASTContext &Ctx) { TraverseAST(Ctx); }
const std::unordered_set<const CallExpr *> &
getFnCalls(const FunctionDecl *Fn) {
@@ -136,8 +136,7 @@
auto MyDiag = diag(Param->getLocation(), "parameter %0 is unused") << Param;
if (!Indexer) {
- Indexer = llvm::make_unique<IndexerVisitor>(
- Result.Context->getTranslationUnitDecl());
+ Indexer = llvm::make_unique<IndexerVisitor>(*Result.Context);
}
// Comment out parameter name for non-local functions.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
