[clang] 0fb4ef4 - Revert "[clang-tidy] Avoid processing declarations in system headers … (#132743)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 24 12:48:02 PDT 2025
Author: Carlos Galvez
Date: 2025-03-24T20:47:57+01:00
New Revision: 0fb4ef40b12d08e4a4966eeac6a395b669d63237
URL: https://github.com/llvm/llvm-project/commit/0fb4ef40b12d08e4a4966eeac6a395b669d63237
DIFF: https://github.com/llvm/llvm-project/commit/0fb4ef40b12d08e4a4966eeac6a395b669d63237.diff
LOG: Revert "[clang-tidy] Avoid processing declarations in system headers … (#132743)
…(#128150)"
This was too aggressive, and leads to problems for downstream users:
https://github.com/llvm/llvm-project/pull/128150#issuecomment-2739803409
Let's revert and reland it once we have addressed the problems.
This reverts commit e4a8969e56572371201863594b3a549de2e23f32.
Co-authored-by: Carlos Gálvez <carlos.galvez at zenseact.com>
Added:
Modified:
clang-tools-extra/clang-query/Query.cpp
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp
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
clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-query/Query.cpp b/clang-tools-extra/clang-query/Query.cpp
index 091713600686e..382aa5d6fe25e 100644
--- a/clang-tools-extra/clang-query/Query.cpp
+++ b/clang-tools-extra/clang-query/Query.cpp
@@ -114,7 +114,7 @@ bool MatchQuery::run(llvm::raw_ostream &OS, QuerySession &QS) const {
Profiler.emplace();
for (auto &AST : QS.ASTs) {
- ast_matchers::MatchFinderOptions FinderOptions;
+ ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
std::optional<llvm::StringMap<llvm::TimeRecord>> Records;
if (QS.EnableProfile) {
Records.emplace();
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index d99847a82d168..733a53a0f5dcc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -420,7 +420,7 @@ ClangTidyASTConsumerFactory::createASTConsumer(
std::vector<std::unique_ptr<ClangTidyCheck>> Checks =
CheckFactories->createChecksForLanguage(&Context);
- ast_matchers::MatchFinderOptions FinderOptions;
+ ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
std::unique_ptr<ClangTidyProfiling> Profiling;
if (Context.getEnableProfiling()) {
@@ -429,10 +429,6 @@ 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.SkipSystemHeaders = true;
-
std::unique_ptr<ast_matchers::MatchFinder> Finder(
new ast_matchers::MatchFinder(std::move(FinderOptions)));
diff --git a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
index 2dff4c0e53b8c..bc4970825b4ca 100644
--- a/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,30 +35,6 @@ 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 {
@@ -66,10 +42,12 @@ namespace clang::tidy::cert {
void DontModifyStdNamespaceCheck::registerMatchers(MatchFinder *Finder) {
auto HasStdParent =
hasDeclContext(namespaceDecl(hasAnyName("std", "posix"),
- unless(hasDeclContext(namespaceDecl())))
+ unless(hasParent(namespaceDecl())))
.bind("nmspc"));
- auto UserDefinedType = qualType(hasUnqualifiedDesugaredType(
- tagType(unless(hasDeclaration(tagDecl(isInStdOrPosixNS()))))));
+ auto UserDefinedType = qualType(
+ hasUnqualifiedDesugaredType(tagType(unless(hasDeclaration(tagDecl(
+ hasAncestor(namespaceDecl(hasAnyName("std", "posix"),
+ unless(hasParent(namespaceDecl()))))))))));
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 ed7da975f3de7..aa85105918ecf 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,12 +91,6 @@ Improvements to clang-query
Improvements to clang-tidy
--------------------------
-- :program:`clang-tidy` no longer processes declarations from system headers
- by default, greatly improving performance. This behavior is disabled if the
- `SystemHeaders` option is enabled.
- Note: this may lead to false negatives; downstream users may need to adjust
- their checks to preserve existing behavior.
-
- Improved :program:`clang-tidy-
diff .py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
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 ad6525276ff8a..1b4d4e924a721 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,29 +33,23 @@
// RUN: readability-identifier-naming.LocalConstantPointerPrefix: 'lc_', \
// RUN: }}'
-// FIXME: make this test case pass.
-// Currently not working because the CXXRecordDecl for the global anonymous
-// union is *not* collected as a top-level declaration.
-// https://github.com/llvm/llvm-project/issues/130618
-#if 0
static union {
int global;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
-// FIXME-CHECK-FIXES: {{^}} int g_global;{{$}}
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'global'
+// CHECK-FIXES: {{^}} int g_global;{{$}}
const int global_const;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for global constant 'global_const'
-// FIXME-CHECK-FIXES: {{^}} 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;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for global pointer 'global_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *GlobalPtr_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;
-// FIXME-CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global constant pointer 'global_const_ptr'
-// FIXME-CHECK-FIXES: {{^}} int *const GLOBAL_CONST_PTR_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;{{$}}
};
-#endif
namespace ns {
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 a7956b4599b4f..448ef9ddf166c 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -66,12 +66,19 @@ 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 a25480e9aa39c..9fa990b6aac8c 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 --allow-empty %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='.*' -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 --allow-empty %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
#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 8182bccdd2da8..40d6785bd2f85 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -458,11 +458,6 @@ AST Matchers
- Ensure ``isDerivedFrom`` matches the correct base in case more than one alias exists.
- Extend ``templateArgumentCountIs`` to support function and variable template
specialization.
-- Move ``ast_matchers::MatchFinder::MatchFinderOptions`` to
- ``ast_matchers::MatchFinderOptions``.
-- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``, and make
- ``MatchASTConsumer`` receive a reference to ``MatchFinderOptions`` in the
- constructor. This allows it to skip 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 a6a8a350bfcd0..a387d9037b7da 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchFinder.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchFinder.h
@@ -50,24 +50,6 @@ namespace clang {
namespace ast_matchers {
-/// A struct defining options for configuring the MatchFinder.
-struct MatchFinderOptions {
- struct Profiling {
- Profiling(llvm::StringMap<llvm::TimeRecord> &Records) : Records(Records) {}
-
- /// Per bucket timing information.
- llvm::StringMap<llvm::TimeRecord> &Records;
- };
-
- /// Enables per-check timers.
- ///
- /// It prints a report after match.
- std::optional<Profiling> CheckProfiling;
-
- /// Avoids matching declarations in system headers.
- bool SkipSystemHeaders = false;
-};
-
/// A class to allow finding matches over the Clang AST.
///
/// After creation, you can add multiple matchers to the MatchFinder via
@@ -144,6 +126,21 @@ class MatchFinder {
virtual void run() = 0;
};
+ struct MatchFinderOptions {
+ struct Profiling {
+ Profiling(llvm::StringMap<llvm::TimeRecord> &Records)
+ : Records(Records) {}
+
+ /// Per bucket timing information.
+ llvm::StringMap<llvm::TimeRecord> &Records;
+ };
+
+ /// Enables per-check timers.
+ ///
+ /// It prints a report after match.
+ std::optional<Profiling> CheckProfiling;
+ };
+
MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
~MatchFinder();
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index e347d0c54d9b0..e9ec7eff1e0ab 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -28,7 +28,6 @@
#include <deque>
#include <memory>
#include <set>
-#include <vector>
namespace clang {
namespace ast_matchers {
@@ -423,7 +422,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
public ASTMatchFinder {
public:
MatchASTVisitor(const MatchFinder::MatchersByType *Matchers,
- const MatchFinderOptions &Options)
+ const MatchFinder::MatchFinderOptions &Options)
: Matchers(Matchers), Options(Options), ActiveASTContext(nullptr) {}
~MatchASTVisitor() override {
@@ -1351,7 +1350,7 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
/// We precalculate a list of matchers that pass the toplevel restrict check.
llvm::DenseMap<ASTNodeKind, std::vector<unsigned short>> MatcherFiltersMap;
- const MatchFinderOptions &Options;
+ const MatchFinder::MatchFinderOptions &Options;
ASTContext *ActiveASTContext;
// Maps a canonical type to its TypedefDecls.
@@ -1574,41 +1573,19 @@ bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) {
class MatchASTConsumer : public ASTConsumer {
public:
MatchASTConsumer(MatchFinder *Finder,
- MatchFinder::ParsingDoneTestCallback *ParsingDone,
- const MatchFinderOptions &Options)
- : Finder(Finder), ParsingDone(ParsingDone), Options(Options) {}
+ MatchFinder::ParsingDoneTestCallback *ParsingDone)
+ : Finder(Finder), ParsingDone(ParsingDone) {}
private:
- bool HandleTopLevelDecl(DeclGroupRef DG) override {
- if (Options.SkipSystemHeaders) {
- for (Decl *D : DG) {
- if (!isInSystemHeader(D))
- TraversalScope.push_back(D);
- }
- }
- return true;
- }
-
void HandleTranslationUnit(ASTContext &Context) override {
- if (!TraversalScope.empty())
- Context.setTraversalScope(TraversalScope);
-
if (ParsingDone != nullptr) {
ParsingDone->run();
}
Finder->matchAST(Context);
}
- bool isInSystemHeader(Decl *D) {
- const SourceManager &SM = D->getASTContext().getSourceManager();
- const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc());
- return SM.isInSystemHeader(Loc);
- }
-
MatchFinder *Finder;
MatchFinder::ParsingDoneTestCallback *ParsingDone;
- const MatchFinderOptions &Options;
- std::vector<Decl *> TraversalScope;
};
} // end namespace
@@ -1727,8 +1704,7 @@ bool MatchFinder::addDynamicMatcher(const internal::DynTypedMatcher &NodeMatch,
}
std::unique_ptr<ASTConsumer> MatchFinder::newASTConsumer() {
- return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone,
- Options);
+ return std::make_unique<internal::MatchASTConsumer>(this, ParsingDone);
}
void MatchFinder::match(const clang::DynTypedNode &Node, ASTContext &Context) {
diff --git a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
index 539a5877a8c4f..a930638f355b9 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -198,7 +198,7 @@ TEST(AstPolymorphicMatcherPMacro, Works) {
}
TEST(MatchFinder, CheckProfiling) {
- MatchFinderOptions Options;
+ MatchFinder::MatchFinderOptions Options;
llvm::StringMap<llvm::TimeRecord> Records;
Options.CheckProfiling.emplace(Records);
MatchFinder Finder(std::move(Options));
More information about the cfe-commits
mailing list