Skip to content

Revert "[clang-tidy] Avoid processing declarations in system headers … #132743

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang-tools-extra/clang-query/Query.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
6 changes: 1 addition & 5 deletions clang-tools-extra/clang-tidy/ClangTidy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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)));

Expand Down
32 changes: 5 additions & 27 deletions clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,41 +35,19 @@ 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 {

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(
Expand Down
6 changes: 0 additions & 6 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 0 additions & 5 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
------------
Expand Down
33 changes: 15 additions & 18 deletions clang/include/clang/ASTMatchers/ASTMatchFinder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down
34 changes: 5 additions & 29 deletions clang/lib/ASTMatchers/ASTMatchFinder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
#include <deque>
#include <memory>
#include <set>
#include <vector>

namespace clang {
namespace ast_matchers {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down