Author: Carlos Galvez Date: 2025-08-17T11:40:48+02:00 New Revision: bd77e9acf0e86a46379e1780dd58a787a7ee78f5
URL: https://github.com/llvm/llvm-project/commit/bd77e9acf0e86a46379e1780dd58a787a7ee78f5 DIFF: https://github.com/llvm/llvm-project/commit/bd77e9acf0e86a46379e1780dd58a787a7ee78f5.diff LOG: [clang-tidy] Avoid matching nodes in system headers (#151035) This commit is a re-do of e4a8969e56572371201863594b3a549de2e23f32, which got reverted, with the same goal: dramatically speed-up clang-tidy by avoiding doing work in system headers (which is wasteful as warnings are later discarded). This proposal was already discussed here with favorable feedback: https://github.com/llvm/llvm-project/pull/132725 The novelty of this patch is: - It's less aggressive: it does not fiddle with AST traversal. This solves the issue with the previous patch, which impacted the ability to inspect parents of a given node. - Instead, what we optimize for is exitting early in each `Traverse*` function of `MatchASTVisitor` if the node is in a system header, thus avoiding calling the `match()` function with its corresponding callback (when there is a match). - It does not cause any failing tests. - It does not move `MatchFinderOptions` - instead we add a user-defined default constructor which solves the same problem. - It introduces a function `shouldSkipNode` which can be extended for adding more conditions. For example there's a PR open about skipping modules in clang-tidy where this could come handy: https://github.com/llvm/llvm-project/pull/145630 As a benchmark, I ran clang-tidy with all checks activated, on a single .cpp file which #includes all the standard C++ headers, then measure the time as well as found warnings. On trunk: ``` Suppressed 75413 warnings (75413 in non-user code). real 0m12.418s user 0m12.270s sys 0m0.129s ``` With this patch: ``` Suppressed 11448 warnings (11448 in non-user code). Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well. real 0m1.666s user 0m1.538s sys 0m0.129s ``` With the original patch that got reverted: ``` Suppressed 11428 warnings (11428 in non-user code). real 0m1.193s user 0m1.096s sys 0m0.096s ``` We therefore get a dramatic reduction in number of warnings and runtime, with no change in functionality. The remaining warnings are due to `PPCallbacks` - implementing a similar system-header exclusion mechanism there can lead to almost no warnings left in system headers. This does not bring the runtime down as much, though, so it's probably not worth the effort. Fixes #52959 Co-authored-by: Carlos Gálvez <carlos.gal...@zenseact.com> Added: Modified: clang-tools-extra/clang-tidy/ClangTidy.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp clang/docs/ReleaseNotes.rst clang/include/clang/ASTMatchers/ASTMatchFinder.h clang/lib/ASTMatchers/ASTMatchFinder.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 4ae2864d310d0..b612d4f18accb 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -424,6 +424,10 @@ ClangTidyASTConsumerFactory::createASTConsumer( FinderOptions.CheckProfiling.emplace(Profiling->Records); } + // Avoid processing system headers, unless the user explicitly requests it + if (!Context.getOptions().SystemHeaders.value_or(false)) + FinderOptions.IgnoreSystemHeaders = true; + std::unique_ptr<ast_matchers::MatchFinder> Finder( new ast_matchers::MatchFinder(std::move(FinderOptions))); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1553f461634d0..36703dd32c03c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,10 @@ Improvements to clang-query Improvements to clang-tidy -------------------------- +- :program:`clang-tidy` no longer attemps to analyze code from system headers + by default, greatly improving performance. This behavior is disabled if the + `SystemHeaders` option is enabled. + - The :program:`run-clang-tidy.py` and :program:`clang-tidy- diff .py` scripts now run checks in parallel by default using all available hardware threads. Both scripts display the number of threads being used in their output. diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp index 448ef9ddf166c..d9ec1049963b0 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp @@ -66,19 +66,14 @@ class A { A(int); }; // CHECK4-NOT: warning: // CHECK4-QUIET-NOT: warning: -// CHECK: Suppressed 3 warnings (3 in non-user code) // CHECK: Use -header-filter=.* to display errors from all non-system headers. // CHECK-QUIET-NOT: Suppressed -// CHECK2: Suppressed 1 warnings (1 in non-user code) -// CHECK2: Use -header-filter=.* {{.*}} // CHECK2-QUIET-NOT: Suppressed -// CHECK3: Suppressed 2 warnings (2 in non-user code) // CHECK3: Use -header-filter=.* {{.*}} // CHECK3-QUIET-NOT: Suppressed // CHECK4-NOT: Suppressed {{.*}} warnings // CHECK4-NOT: Use -header-filter=.* {{.*}} // CHECK4-QUIET-NOT: Suppressed -// CHECK6: Suppressed 2 warnings (2 in non-user code) // CHECK6: Use -header-filter=.* {{.*}} int x = 123; diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp index 9fa990b6aac8c..a25480e9aa39c 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp @@ -11,9 +11,9 @@ // RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s -// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s +// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s -// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s +// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s #include <system_header.h> // CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 604b4c3f714b7..b35f4ea42818a 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -308,6 +308,9 @@ AST Matchers - Ensure ``hasBitWidth`` doesn't crash on bit widths that are dependent on template parameters. +- Add a boolean member ``IgnoreSystemHeaders`` to ``MatchFinderOptions``. This + allows it to ignore nodes in system headers when traversing the AST. + clang-format ------------ diff --git a/clang/include/clang/ASTMatchers/ASTMatchFinder.h b/clang/include/clang/ASTMatchers/ASTMatchFinder.h index 73cbcf1f25025..2d36e8c4fae1c 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchFinder.h +++ b/clang/include/clang/ASTMatchers/ASTMatchFinder.h @@ -135,10 +135,15 @@ class MatchFinder { llvm::StringMap<llvm::TimeRecord> &Records; }; + MatchFinderOptions() {} + /// Enables per-check timers. /// /// It prints a report after match. std::optional<Profiling> CheckProfiling; + + /// Avoids matching declarations in system headers. + bool IgnoreSystemHeaders{false}; }; MatchFinder(MatchFinderOptions Options = MatchFinderOptions()); diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp index d43d1aec71b29..e8a0004c2e187 100644 --- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp +++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp @@ -1344,6 +1344,41 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>, return false; } + template <typename T> static SourceLocation getNodeLocation(const T &Node) { + return Node.getBeginLoc(); + } + + static SourceLocation getNodeLocation(const CXXCtorInitializer &Node) { + return Node.getSourceLocation(); + } + + static SourceLocation getNodeLocation(const TemplateArgumentLoc &Node) { + return Node.getLocation(); + } + + static SourceLocation getNodeLocation(const Attr &Node) { + return Node.getLocation(); + } + + bool isInSystemHeader(SourceLocation Loc) { + const SourceManager &SM = getASTContext().getSourceManager(); + return SM.isInSystemHeader(Loc); + } + + template <typename T> bool shouldSkipNode(T &Node) { + if (Options.IgnoreSystemHeaders && isInSystemHeader(getNodeLocation(Node))) + return true; + return false; + } + + template <typename T> bool shouldSkipNode(T *Node) { + return (Node == nullptr) || shouldSkipNode(*Node); + } + + bool shouldSkipNode(QualType &) { return false; } + + bool shouldSkipNode(NestedNameSpecifier &) { return false; } + /// Bucket to record map. /// /// Used to get the appropriate bucket for each matcher. @@ -1473,9 +1508,8 @@ bool MatchASTVisitor::objcClassIsDerivedFrom( } bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) { - if (!DeclNode) { + if (shouldSkipNode(DeclNode)) return true; - } bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit(); @@ -1503,9 +1537,9 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) { } bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) { - if (!StmtNode) { + if (shouldSkipNode(StmtNode)) return true; - } + bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || TraversingASTChildrenNotSpelledInSource; @@ -1515,6 +1549,9 @@ bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) { } bool MatchASTVisitor::TraverseType(QualType TypeNode, bool TraverseQualifier) { + if (shouldSkipNode(TypeNode)) + return true; + match(TypeNode); return RecursiveASTVisitor<MatchASTVisitor>::TraverseType(TypeNode, TraverseQualifier); @@ -1522,6 +1559,8 @@ bool MatchASTVisitor::TraverseType(QualType TypeNode, bool TraverseQualifier) { bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode, bool TraverseQualifier) { + if (shouldSkipNode(TypeLocNode)) + return true; // The RecursiveASTVisitor only visits types if they're not within TypeLocs. // We still want to find those types via matchers, so we match them here. Note // that the TypeLocs are structurally a shadow-hierarchy to the expressed @@ -1534,6 +1573,9 @@ bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode, } bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier NNS) { + if (shouldSkipNode(NNS)) + return true; + match(NNS); return RecursiveASTVisitor<MatchASTVisitor>::TraverseNestedNameSpecifier(NNS); } @@ -1543,6 +1585,9 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc( if (!NNS) return true; + if (shouldSkipNode(NNS)) + return true; + match(NNS); // We only match the nested name specifier here (as opposed to traversing it) @@ -1555,7 +1600,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc( bool MatchASTVisitor::TraverseConstructorInitializer( CXXCtorInitializer *CtorInit) { - if (!CtorInit) + if (shouldSkipNode(CtorInit)) return true; bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || @@ -1573,11 +1618,17 @@ bool MatchASTVisitor::TraverseConstructorInitializer( } bool MatchASTVisitor::TraverseTemplateArgumentLoc(TemplateArgumentLoc Loc) { + if (shouldSkipNode(Loc)) + return true; + match(Loc); return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateArgumentLoc(Loc); } bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) { + if (shouldSkipNode(AttrNode)) + return true; + match(*AttrNode); return RecursiveASTVisitor<MatchASTVisitor>::TraverseAttr(AttrNode); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
