llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Carlos Galvez (carlosgalvezp) <details> <summary>Changes</summary> …atchFinder This was introduced here with the purpose of speed up clang-tidy, making it not process system headers: https://github.com/llvm/llvm-project/commit/e4a8969e56572371201863594b3a549de2e23f32 However it was later realized that this change is too aggressive: the reduced traversal scope also impacts the ParentMapContext, which in turn means that matchers like "hasParent" or "hasAncestor" are not able to find parents in system headers, even if the declaration at hand is in a user header. This causes regressions for downstream users writing custom clang-tidy checks: https://github.com/llvm/llvm-project/pull/128150#issuecomment-2739803409 This patch fixes this problem, as follows: - Revert the changes to the clang-tidy unit tests. - Revert setting the TraversalScope in MatchASTConsumer. - Move the logic to MatchASTVisitor::TraverseDecl instead. Pros: - Leaves the ASTContext and ParentMapContext untouched, so hasParent and similar matchers should work again. - Keeps avoiding matching and checking decls outside of system headers. - The broken unit test in readability now works, because we are no longer processing only TopLevelDecls. - The changes to the CERT test can be reverted. - Most likely we don't lose any functionality or introduce false negatives. Cons: - We still have to traverse decls in system headers. I did try to return false; instead of return true; in the proposed code but that made around 60 clang-tidy tests to fail. - We still process many nodes (not Decls) that are in system headers. As a benchmark, I tried running all clang-tidy checks on a .cpp file that includes all the standard C++ headers. This leads to: * Baseline (without any optimizations). Suppressed 196093 warnings (196093 in non-user code) 10 seconds to run. * Trunk (aggressive optimizations). Suppressed 8050 warnings (8050 in non-user code). 1 second to run. * This patch (conservative optimizations). Suppressed 141779 warnings (141779 in non-user code). 3 seconds to run. This patch thus gives some performance improvement while keeping backwards compatibility. There's still room for improvement which shall be tackled in a follow-up patch after unbreaking clang-tidy. Fixes #<!-- -->130618 --- Full diff: https://github.com/llvm/llvm-project/pull/132725.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp (+5-27) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (-2) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp (+8-14) - (modified) clang/docs/ReleaseNotes.rst (+2-3) - (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+13-27) ``````````diff 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..a05be39c8dce5 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -94,8 +94,6 @@ 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/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 8182bccdd2da8..f4698b6c70c81 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -460,9 +460,8 @@ AST Matchers 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. +- Add a boolean member ``SkipSystemHeaders`` to ``MatchFinderOptions``. This + allows it to avoid matching declarations in system headers . clang-format ------------ diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp index e347d0c54d9b0..995d9986b1ba3 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 { @@ -1464,11 +1463,21 @@ bool MatchASTVisitor::objcClassIsDerivedFrom( return false; } +static bool isInSystemHeader(Decl *D) { + const SourceManager &SM = D->getASTContext().getSourceManager(); + const SourceLocation Loc = SM.getExpansionLoc(D->getBeginLoc()); + return SM.isInSystemHeader(Loc); +} + bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) { if (!DeclNode) { return true; } + // Skip declarations in system headers if requested + if (Options.SkipSystemHeaders && isInSystemHeader(DeclNode)) + return true; + bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit(); bool ScopedChildren = TraversingASTChildrenNotSpelledInSource; @@ -1574,41 +1583,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 +1714,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) { `````````` </details> https://github.com/llvm/llvm-project/pull/132725 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
