[clang-tools-extra] [clang-tidy] Avoid processing declarations in system headers (PR #128150)
Carlos Galvez via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 28 00:17:19 PST 2025
Carlos =?utf-8?q?Gálvez?= <carlos.galvez at zenseact.com>,
Carlos =?utf-8?q?Gálvez?= <carlos.galvez at zenseact.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/128150 at github.com>
https://github.com/carlosgalvezp updated https://github.com/llvm/llvm-project/pull/128150
>From 1c3cd94ba1723125a065f69ec1ab689d3f9c2474 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20G=C3=A1lvez?= <carlos.galvez at zenseact.com>
Date: Thu, 20 Feb 2025 12:37:15 +0000
Subject: [PATCH 1/3] [clang-tidy] Avoid processing declarations in system
headers
Currently, clang-tidy processes the entire TranslationUnit, including
declarations in system headers. However, the work done in system
headers is discarded at the very end when presenting results, unless
the SystemHeaders option is active.
This is a lot of wasted work, and makes clang-tidy very slow.
In comparison, clangd only processes declarations in the main file,
and it's claimed to be 10x faster than clang-tidy:
https://github.com/lljbash/clangd-tidy
To solve this problem, we can apply a similar solution done in clangd
into clang-tidy. We do this by changing the traversal scope from the
default TranslationUnitDecl, to only contain the top-level declarations
that are _not_ part of system headers. We do this by prepending a new
ASTConsumer to the list of consumers: this new consumer sets the
traversal scope in the ASTContext, which is later used by the
MatchASTConsumer.
Note: this behavior is not active if the user requests warnings from
system headers via the SystemHeaders option.
Note2: out of all the unit tests, only one of them fails:
readability/identifier-naming-anon-record-fields.cpp
This is because the limited traversal scope no longer includes the
"IndirectFieldDecl" that appears in the AST when having a global
scope anonymous union.
I have not found a way to make this one work. However, it does seem
like a very niche use case, and the benefits of a 10x faster clang-tidy
largely outweigh the false negative now introduced by this patch. This
use case is therefore removed from the unit test to make it pass.
Note3: I have purposely decided to make this new feature enabled by
default, instead of adding a new "opt-in/opt-out" flag. Having a new
flag would mean duplicating all our tests to ensure they work in both
modes, which would be infeasible. Having it enabled by default allow
people to get the benefits immediately. Given that all unit tests pass,
the risk for regressions is low. Even if that's the case, the only
issue would be false negatives (fewer things are detected), which
are much more tolerable than false positives.
Credits: original implementation by @njames93, here:
https://reviews.llvm.org/D150126
This implementation is simpler in the sense that it does not consider
HeaderFilterRegex to filter even further. A follow-up patch could
include the functionality if wanted.
Fixes #52959
---
clang-tools-extra/clang-tidy/ClangTidy.cpp | 33 +++++++++++++++++++
.../cert/DontModifyStdNamespaceCheck.cpp | 32 +++++++++++++++---
clang-tools-extra/docs/ReleaseNotes.rst | 4 +++
.../identifier-naming-anon-record-fields.cpp | 18 ----------
.../clang-tidy/infrastructure/file-filter.cpp | 7 ----
.../infrastructure/system-headers.cpp | 4 +--
6 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 733a53a0f5dcc..40adfe9af74e3 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -339,6 +339,35 @@ class ClangTidyASTConsumer : public MultiplexConsumer {
void anchor() override {};
};
+/// ASTConsumer that filters top-level declarations that are in system headers,
+/// and sets the AST traversal scope to only cover the declarations in user
+/// headers. This makes all clang-tidy checks avoid spending time processing
+/// declarations in system headers. The results are discarded anyway when
+/// presenting the results.
+class IgnoreSystemHeadersConsumer : public ASTConsumer {
+public:
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ for (Decl *D : DG) {
+ if (!isInSystemHeader(D))
+ Decls.push_back(D);
+ }
+ return true;
+ }
+
+ void HandleTranslationUnit(ASTContext &Ctx) override {
+ Ctx.setTraversalScope(Decls);
+ }
+
+private:
+ std::vector<Decl *> Decls;
+
+ bool isInSystemHeader(Decl *D) {
+ SourceManager &SM = D->getASTContext().getSourceManager();
+ SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+ return SM.isInSystemHeader(Loc);
+ }
+};
+
} // namespace
ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
@@ -449,6 +478,10 @@ ClangTidyASTConsumerFactory::createASTConsumer(
}
std::vector<std::unique_ptr<ASTConsumer>> Consumers;
+
+ if (!Context.getOptions().SystemHeaders.value_or(false))
+ Consumers.push_back(std::make_unique<IgnoreSystemHeadersConsumer>());
+
if (!Checks.empty())
Consumers.push_back(Finder->newASTConsumer());
diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index bc4970825b4ca..2dff4c0e53b8c 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,6 +35,30 @@ AST_POLYMORPHIC_MATCHER_P(
Builder) != Args.end();
}
+bool isStdOrPosixImpl(const DeclContext *Ctx) {
+ if (!Ctx->isNamespace())
+ return false;
+
+ const auto *ND = cast<NamespaceDecl>(Ctx);
+ if (ND->isInline()) {
+ return isStdOrPosixImpl(ND->getParent());
+ }
+
+ if (!ND->getParent()->getRedeclContext()->isTranslationUnit())
+ return false;
+
+ const IdentifierInfo *II = ND->getIdentifier();
+ return II && (II->isStr("std") || II->isStr("posix"));
+}
+
+AST_MATCHER(Decl, isInStdOrPosixNS) {
+ for (const auto *Ctx = Node.getDeclContext(); Ctx; Ctx = Ctx->getParent()) {
+ if (isStdOrPosixImpl(Ctx))
+ return true;
+ }
+ return false;
+}
+
} // namespace
namespace clang::tidy::cert {
@@ -42,12 +66,10 @@ namespace clang::tidy::cert {
void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
auto HasStdParent =
hasDeclContext(namespaceDecl(hasAnyName("std", "posix"),
- unless(hasParent(namespaceDecl())))
+ unless(hasDeclContext(namespaceDecl())))
.bind("nmspc"));
- auto UserDefinedType = qualType(
- hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl(
- hasAncestor(namespaceDecl(hasAnyName("std", "posix"),
- unless(hasParent(namespaceDecl()))))))))));
+ auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(
+ tagType(unless(hasDeclaration(tagDecl(isInStdOrPosixNS()))))));
auto HasNoProgramDefinedTemplateArgument = unless(
hasAnyTemplateArgumentIncludingPack(refersToType(UserDefinedType)));
auto InsideStdClassOrClassTemplateSpecialization = hasDeclContext(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a8d17d19fda1d..5070be7d72d09 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -88,6 +88,10 @@ Improvements to clang-query
Improvements to clang-tidy
--------------------------
+- It no longer processes declarations from system headers by default, greatly
+ improving performance (up to 10x speed-up). This behavior is disabled if the
+ `SystemHeaders` option is enabled.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
index 1b4d4e924a721..2604c88a30efb 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
@@ -33,24 +33,6 @@
// RUN: readability-identifier-naming.LocalConstantPointerPrefix: 'lc_', \
// RUN: }}'
-static union {
- int global;
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
-// CHECK-FIXES: {{^}} int g_global;{{$}}
-
- const int global_const;
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
-// CHECK-FIXES: {{^}} const int GLOBAL_CONST;{{$}}
-
- int *global_ptr;
-// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
-// CHECK-FIXES: {{^}} int *GlobalPtr_Ptr;{{$}}
-
- int *const global_const_ptr;
-// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
-// CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_Ptr;{{$}}
-};
-
namespace ns {
static union {
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..a7956b4599b4f 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,12 @@ 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
>From bfff449b14a7bb07bb03d999e4571af338ee266f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20G=C3=A1lvez?= <carlos.galvez at zenseact.com>
Date: Fri, 21 Feb 2025 19:20:12 +0000
Subject: [PATCH 2/3] Fix review comments
---
clang-tools-extra/clang-tidy/ClangTidy.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 40adfe9af74e3..619931f49f469 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -42,6 +42,7 @@
#include "llvm/Support/Process.h"
#include <algorithm>
#include <utility>
+#include <vector>
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
#include "clang/Analysis/PathDiagnostic.h"
@@ -362,8 +363,8 @@ class IgnoreSystemHeadersConsumer : public ASTConsumer {
std::vector<Decl *> Decls;
bool isInSystemHeader(Decl *D) {
- SourceManager &SM = D->getASTContext().getSourceManager();
- SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
+ const SourceManager &SM = D->getASTContext().getSourceManager();
+ const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
return SM.isInSystemHeader(Loc);
}
};
>From 2cc16908e8ffeecca4f9698f13bf5d40f90c42cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20G=C3=A1lvez?= <carlos.galvez at zenseact.com>
Date: Fri, 21 Feb 2025 19:21:31 +0000
Subject: [PATCH 3/3] Remove claim about 10x speedup
That kind of speedup is only achievable if also the user headers are
excluded from analysis. A follow-up patch could make clang-tidy
ignore declarations on files not matched by the HeaderFilter option.
---
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5070be7d72d09..3b301f5c6239d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -89,8 +89,8 @@ Improvements to clang-tidy
--------------------------
- It no longer processes declarations from system headers by default, greatly
- improving performance (up to 10x speed-up). This behavior is disabled if the
- `SystemHeaders` option is enabled.
+ improving performance. This behavior is disabled if the `SystemHeaders` option
+ is enabled.
New checks
^^^^^^^^^^
More information about the cfe-commits
mailing list