njames93 created this revision.
njames93 added reviewers: carlosgalvezp, PiotrZSL.
Herald added a subscriber: xazax.hun.
Herald added a project: All.
njames93 updated this revision to Diff 520620.
njames93 added a comment.
njames93 updated this revision to Diff 520918.
njames93 retitled this revision from "[clang-tidy] Set traversal scope to
prevent analysis in headers" to "[clang-tidy][WIP] Set traversal scope to
prevent analysis in headers".
njames93 published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Fix failing unittests
njames93 added a comment.
Fix infrastructure test failures
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:511
+
+ Consumers.push_back(std::make_unique<TraversalScopeConsumer>(
+ !Context.getOptions().SystemHeaders.value_or(false),
----------------
Here comes a stupid question :) If I understand correctly we are adding one
more ASTConsumer to the list of Consumers, i.e. we are not replacing the
"normal" consumer with the "optimized" consumer. So I fail to understand how
adding this new consumer fixes the problems of the other consumer? I'd be happy
to read more about the implementation if there are resources you could point me
to.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:511
+
+ Consumers.push_back(std::make_unique<TraversalScopeConsumer>(
+ !Context.getOptions().SystemHeaders.value_or(false),
----------------
carlosgalvezp wrote:
> Here comes a stupid question :) If I understand correctly we are adding one
> more ASTConsumer to the list of Consumers, i.e. we are not replacing the
> "normal" consumer with the "optimized" consumer. So I fail to understand how
> adding this new consumer fixes the problems of the other consumer? I'd be
> happy to read more about the implementation if there are resources you could
> point me to.
The `MultiplexConsumer` runs each consumers callbacks in the order they are
added. So this consumers `HandleTranslationUnit` callback will be executed
before the `FinderASTConsumer `runs its `HandleTranslationUnit` callback(The
function that actually does the AST matching. So by modifying the `ASTContext`
traversal scope in this `HandleTranslationUnit` callback, the results are
visible when the Finder comes to do its matching.
Also part of this implementation was taken from clangd which uses a somewhat
similar approach to only run tidy checks that originate in the main file
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ParsedAST.cpp#L73
See
https://discourse.llvm.org/t/rfc-exclude-issues-from-system-headers-as-early-as-posible/68483
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D150126
Files:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp
+++ 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 %s --allow-empty
// 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 %s --allow-empty
#include <system_header.h>
// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
Index: clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/line-filter.cpp
@@ -24,4 +24,4 @@
// CHECK-NOT: warning:
-// CHECK: Suppressed 3 warnings (1 in non-user code, 2 due to line filter)
+// CHECK: Suppressed 2 warnings (2 due to line filter)
Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -61,14 +61,8 @@
// 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=.* {{.*}}
Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter-symlinks.cpp
@@ -9,8 +9,8 @@
// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='dir1/dir2/\.\./header_alias\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER_ALIAS %s
// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header_alias\.h' %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER_ALIAS %s
// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header_alias\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER_ALIAS %s
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s --allow-empty
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header\.h' -quiet %s -- -I %t 2>&1 | FileCheck --check-prefix=CHECK_HEADER %s --allow-empty
// Check that `-header-filter` operates on the same file paths as paths in
// diagnostics printed by ClangTidy.
Index: clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
+++ clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
@@ -35,6 +35,30 @@
Builder) != Args.end();
}
+static 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 @@
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(
Index: clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
@@ -34,7 +34,7 @@
0, expr(hasType(cxxRecordDecl(hasName("::absl::Duration"))))),
hasArgument(1, expr().bind("arg")),
callee(functionDecl(
- hasParent(functionTemplateDecl()),
+ ast_matchers::isTemplateInstantiation(),
unless(hasTemplateArgument(0, refersToType(builtinType()))),
hasAnyName("operator*=", "operator/="))))
.bind("OuterExpr"),
@@ -46,7 +46,7 @@
cxxMemberCallExpr(
callee(cxxMethodDecl(
ofClass(cxxRecordDecl(hasName("::absl::Duration"))),
- hasParent(functionTemplateDecl()),
+ ast_matchers::isTemplateInstantiation(),
unless(hasTemplateArgument(0, refersToType(builtinType()))),
hasAnyName("operator*=", "operator/="))),
argumentCountIs(1), hasArgument(0, expr().bind("arg")))
@@ -58,7 +58,7 @@
// built-in type.
Finder->addMatcher(
callExpr(callee(functionDecl(
- hasParent(functionTemplateDecl()),
+ ast_matchers::isTemplateInstantiation(),
unless(hasTemplateArgument(0, refersToType(builtinType()))),
hasAnyName("::absl::operator*", "::absl::operator/"))),
argumentCountIs(2),
@@ -72,7 +72,7 @@
// built-in type and `b` has type `absl::Duration`.
Finder->addMatcher(
callExpr(callee(functionDecl(
- hasParent(functionTemplateDecl()),
+ ast_matchers::isTemplateInstantiation(),
unless(hasTemplateArgument(0, refersToType(builtinType()))),
hasName("::absl::operator*"))),
argumentCountIs(2), hasArgument(0, expr().bind("arg")),
@@ -98,16 +98,17 @@
// `absl::Hours(x)`
// where `x` is not of a built-in type.
Finder->addMatcher(
- traverse(TK_AsIs, implicitCastExpr(
- anyOf(hasCastKind(CK_UserDefinedConversion),
- has(implicitCastExpr(
- hasCastKind(CK_UserDefinedConversion)))),
- hasParent(callExpr(
- callee(functionDecl(
- DurationFactoryFunction(),
- unless(hasParent(functionTemplateDecl())))),
- hasArgument(0, expr().bind("arg")))))
- .bind("OuterExpr")),
+ traverse(TK_AsIs,
+ implicitCastExpr(
+ anyOf(hasCastKind(CK_UserDefinedConversion),
+ has(implicitCastExpr(
+ hasCastKind(CK_UserDefinedConversion)))),
+ hasParent(callExpr(
+ callee(functionDecl(
+ DurationFactoryFunction(),
+ unless(ast_matchers::isTemplateInstantiation()))),
+ hasArgument(0, expr().bind("arg")))))
+ .bind("OuterExpr")),
this);
}
Index: clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp
@@ -19,13 +19,13 @@
// TODO: refactor matcher to be configurable or just match on any internal
// access from outside the enclosing namespace.
- Finder->addMatcher(
- nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(
- matchesName("internal"),
- hasParent(namespaceDecl(hasName("absl")))))),
- unless(isInAbseilFile()))
- .bind("InternalDep"),
- this);
+ Finder->addMatcher(nestedNameSpecifierLoc(
+ loc(specifiesNamespace(namespaceDecl(
+ matchesName("internal"),
+ hasDeclContext(namespaceDecl(hasName("absl")))))),
+ unless(isInAbseilFile()))
+ .bind("InternalDep"),
+ this);
}
void NoInternalDependenciesCheck::check(const MatchFinder::MatchResult &Result) {
Index: clang-tools-extra/clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -431,6 +431,87 @@
}
std::vector<std::unique_ptr<ASTConsumer>> Consumers;
+
+ if (!Context.getOptions().SystemHeaders.value_or(false) ||
+ Context.getOptions().HeaderFilterRegex.value_or("") != ".*") {
+ class TraversalScopeConsumer : public ASTConsumer {
+ void Initialize(ASTContext &Context) override {
+ // Make sure the main file ID always gets included.
+ Cache.insert(
+ std::make_pair(Context.getSourceManager().getMainFileID(), true));
+ }
+
+ bool shouldKeepDeclsFromFile(SourceManager &SM, FileID FID) {
+ // Tricky cases, just assume we should include
+ if (FID.isInvalid() || FID == FileID::getSentinel())
+ return true;
+
+ auto [Item, Inserted] = Cache.try_emplace(FID, true);
+
+ // Already in cache, return cached value
+ if (!Inserted)
+ return Item->getSecond();
+
+ // Hash value isn't strictly what we need, but it currently just returns
+ // the opaque id which is what we need. Any other way to get the ID
+ // would require modifying the FID class or type-punning.
+ auto Value = static_cast<int>(FID.getHashValue());
+ SrcMgr::SLocEntry Entry;
+ if (Value > 0) {
+ // Regular file
+ Entry = SM.getLocalSLocEntry(static_cast<unsigned>(Value));
+ } else {
+ // Module
+ bool Invalid = false;
+ Entry = SM.getLoadedSLocEntry(static_cast<unsigned>(-Value - 2),
+ &Invalid);
+ if (Invalid)
+ return true;
+ }
+
+ if (FilterSystem &&
+ SrcMgr::isSystem(Entry.getFile().getFileCharacteristic()))
+ return Item->second = false;
+ if (HeaderFilter)
+ return Item->second = HeaderFilter->match(Entry.getFile().getName());
+ return true;
+ }
+
+ // Gather all top level decls
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ for (auto *D : DG) {
+ auto &SM = D->getASTContext().getSourceManager();
+ if (!shouldKeepDeclsFromFile(
+ SM, SM.getDecomposedExpansionLoc(D->getLocation()).first))
+ continue;
+ Decls.push_back(D);
+ }
+ return true;
+ }
+
+ // Set the contexts traversal scope to only include top-level decls.
+ void HandleTranslationUnit(ASTContext &Ctx) override {
+ Ctx.setTraversalScope(Decls);
+ }
+
+ std::optional<llvm::Regex> HeaderFilter;
+ llvm::DenseMap<clang::FileID, bool> Cache;
+ bool FilterSystem;
+ std::vector<Decl *> Decls;
+
+ public:
+ TraversalScopeConsumer(bool FilterSystem,
+ const std::optional<std::string> &HeaderFilter)
+ : FilterSystem(FilterSystem) {
+ if (HeaderFilter && *HeaderFilter != ".*")
+ this->HeaderFilter.emplace(*HeaderFilter);
+ }
+ };
+
+ Consumers.push_back(std::make_unique<TraversalScopeConsumer>(
+ !Context.getOptions().SystemHeaders.value_or(false),
+ Context.getOptions().HeaderFilterRegex));
+ }
if (!Checks.empty())
Consumers.push_back(Finder->newASTConsumer());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
