llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Carlos Galvez (carlosgalvezp) <details> <summary>Changes</summary> …(#<!-- -->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. --- Full diff: https://github.com/llvm/llvm-project/pull/132743.diff 11 Files Affected: - (modified) clang-tools-extra/clang-query/Query.cpp (+1-1) - (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+1-5) - (modified) clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp (+5-27) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (-6) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-anon-record-fields.cpp (+8-14) - (modified) clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp (+7) - (modified) clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp (+2-2) - (modified) clang/docs/ReleaseNotes.rst (-5) - (modified) clang/include/clang/ASTMatchers/ASTMatchFinder.h (+15-18) - (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+5-29) - (modified) clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp (+1-1) ``````````diff 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)); `````````` </details> https://github.com/llvm/llvm-project/pull/132743 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
