https://github.com/dmaclach updated https://github.com/llvm/llvm-project/pull/212564
>From 3dc8cdffbe79821b459d09846cd22a135f6976da Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Tue, 28 Jul 2026 10:38:28 -0700 Subject: [PATCH 1/6] [clang][include-cleaner] Support ObjC @selector expressions in WalkAST This change adds support for resolving Objective-C @selector expressions to their corresponding method or property declarations. A pre-pass (buildObjCSelectorMap) is introduced to map selectors to their declarations (methods, property getters, and property setters) across the translation unit. When the AST walker encounters an ObjCSelectorExpr, it reports the matching declarations as ambiguous references. --- .../include-cleaner/lib/Analysis.cpp | 47 ++++---- .../include-cleaner/lib/AnalysisInternal.h | 13 ++- .../include-cleaner/lib/HTMLReport.cpp | 46 ++++---- .../include-cleaner/lib/WalkAST.cpp | 62 +++++++++- .../include-cleaner/unittests/WalkASTTest.cpp | 108 ++++++++++++++++-- 5 files changed, 220 insertions(+), 56 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index e48a380211af0..5dfa291573723 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -56,28 +56,33 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; + ObjCSelectorMap SelectorDecls; + if (!ASTRoots.empty()) { + SelectorDecls = buildObjCSelectorMap(ASTRoots.front()->getASTContext()); + } for (auto *Root : ASTRoots) { - walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (RT == RefType::Explicit) - RT = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - // FIXME: Most of the work done here is repetitive. It might be useful to - // have a cache/batching. - SymbolReference SymRef{ND, Loc, RT}; - return CB(SymRef, headersForSymbol(ND, PP, PI)); - }); + walkAST(*Root, SelectorDecls, + [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch + // space and clang currently doesn't have a good/simple APIs for + // tracking where pieces of a concataned token originated from. So + // we use the macro expansion location instead, and downgrade + // reference type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (RT == RefType::Explicit) + RT = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + // FIXME: Most of the work done here is repetitive. It might be + // useful to have a cache/batching. + SymbolReference SymRef{ND, Loc, RT}; + return CB(SymRef, headersForSymbol(ND, PP, PI)); + }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index 7d170fd15014d..c9c3042423a56 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -25,9 +25,12 @@ #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" +#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" #include "clang/Lex/Preprocessor.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/SmallVector.h" #include <vector> namespace clang { @@ -38,6 +41,14 @@ class NamedDecl; class SourceLocation; namespace include_cleaner { +using ObjCSelectorMap = + llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; + +/// Builds a map from Objective-C Selectors to their declarations in the given +/// ASTContext. +/// This is used to optimize selector lookups during AST walking. +ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); + /// Traverses part of the AST from \p Root, finding uses of symbols. /// /// Each use is reported to the callback: @@ -50,7 +61,7 @@ namespace include_cleaner { /// /// walkAST is typically called once per top-level declaration in the file /// being analyzed, in order to find all references within it. -void walkAST(Decl &Root, +void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>); /// Finds the headers that provide the symbol location. diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp index 3e067f84432ac..c7a8b1728925f 100644 --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -503,29 +503,31 @@ void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, llvm::raw_ostream &OS) { Reporter R(OS, Ctx, PP, Includes, PI, File); const auto &SM = Ctx.getSourceManager(); + ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(Ctx); for (Decl *Root : Roots) - walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { - // FIXME: we should merge this logic with `walkUsed` to prevent - // divergences in the future. It isn't trivial though, as we also update - // RefType. Since HTMLReport is only used for debugging purposes, - // divergences aren't critical. - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (T == RefType::Explicit) - T = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - R.addRef(SymbolReference{D, Loc, T}); - }); + walkAST(*Root, SelectorDecls, + [&](SourceLocation Loc, const NamedDecl &D, RefType T) { + // FIXME: we should merge this logic with `walkUsed` to prevent + // divergences in the future. It isn't trivial though, as we also + // update RefType. Since HTMLReport is only used for debugging + // purposes, divergences aren't critical. + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch + // space and clang currently doesn't have a good/simple APIs for + // tracking where pieces of a concataned token originated from. So + // we use the macro expansion location instead, and downgrade + // reference type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (T == RefType::Explicit) + T = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + R.addRef(SymbolReference{D, Loc, T}); + }); for (const SymbolReference &Ref : MacroRefs) { if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) continue; diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 7d15f96405903..03bd3e8f61d00 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -12,9 +12,11 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclFriend.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/ExprObjC.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/TemplateBase.h" #include "clang/AST/TemplateName.h" @@ -24,10 +26,12 @@ #include "clang/Basic/OperatorKinds.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" +#include <utility> namespace clang::include_cleaner { namespace { @@ -39,8 +43,39 @@ bool isOperatorNewDelete(OverloadedOperatorKind OpKind) { using DeclCallback = llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>; +class ObjCSelectorDeclMapBuilder + : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { +public: + ObjCSelectorMap build() && { return std::move(Map); } + + bool TraverseDecl(clang::Decl *D) { + if (!D) + return true; + if (auto *Container = llvm::dyn_cast<clang::ObjCContainerDecl>(D)) { + for (clang::ObjCMethodDecl *M : Container->methods()) { + if (M) { + Map[M->getSelector()].push_back(M); + } + } + for (clang::ObjCPropertyDecl *Prop : Container->properties()) { + if (Prop) { + if (auto Getter = Prop->getGetterName(); !Getter.isNull()) + Map[Getter].push_back(Prop); + if (auto Setter = Prop->getSetterName(); !Setter.isNull()) + Map[Setter].push_back(Prop); + } + } + } + return RecursiveASTVisitor::TraverseDecl(D); + } + +private: + ObjCSelectorMap Map; +}; + class ASTWalker : public RecursiveASTVisitor<ASTWalker> { DeclCallback Callback; + const ObjCSelectorMap &SelectorDecls; void report(SourceLocation Loc, NamedDecl *ND, RefType RT = RefType::Explicit) { @@ -102,7 +137,8 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } public: - ASTWalker(DeclCallback Callback) : Callback(Callback) {} + ASTWalker(DeclCallback Callback, const ObjCSelectorMap &SelectorDecls) + : Callback(Callback), SelectorDecls(SelectorDecls) {} // Operators are almost always ADL extension points and by design references // to them doesn't count as uses (generally the type should provide them, so @@ -479,6 +515,17 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { return true; } + bool VisitObjCSelectorExpr(ObjCSelectorExpr *E) { + auto Sel = E->getSelector(); + auto It = SelectorDecls.find(Sel); + if (It != SelectorDecls.end()) { + for (NamedDecl *ND : It->second) { + report(E->getSelectorNameLoc(), ND, RefType::Ambiguous); + } + } + return true; + } + bool VisitCastExpr(CastExpr *E) { // Handle implicit or explicit casts between Objective-C object pointers // aimed towards protocol-qualification (e.g., `ClassName *` to @@ -572,8 +619,17 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } // namespace -void walkAST(Decl &Root, DeclCallback Callback) { - ASTWalker(Callback).TraverseDecl(&Root); +ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { + ObjCSelectorDeclMapBuilder Builder; + if (Ctx.getLangOpts().ObjC) { + Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); + } + return std::move(Builder).build(); +} + +void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, + DeclCallback Callback) { + ASTWalker(Callback, SelectorDecls).TraverseDecl(&Root); } } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index cf9a5a365edb6..6dd01f44f1223 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -67,18 +67,20 @@ testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode, std::vector<Decl::Kind> TargetDecls; // Perform the walk, and capture the offsets of the referenced targets. std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets; + ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(AST.context()); for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first) continue; - walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - if (SM.getFileLoc(Loc) != ReferencingLoc) - return; - auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); - if (NDLoc.first != TargetFile) - return; - ReferencedOffsets[RT].push_back(NDLoc.second); - TargetDecls.push_back(ND.getKind()); - }); + walkAST(*D, SelectorDecls, + [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + if (SM.getFileLoc(Loc) != ReferencingLoc) + return; + auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); + if (NDLoc.first != TargetFile) + return; + ReferencedOffsets[RT].push_back(NDLoc.second); + TargetDecls.push_back(ND.getKind()); + }); } for (auto &Entry : ReferencedOffsets) llvm::sort(Entry.second); @@ -1164,5 +1166,93 @@ TEST(WalkAST, ObjCIvarRefExprFree) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCSelectorExpr) { + testWalk(R"objc( + @interface MyClass + $ambiguous^- (void)doSomething; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doSomething); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprPropertyGetter) { + testWalk(R"objc( + @interface MyClass + @property(nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^foo); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprPropertySetter) { + testWalk(R"objc( + @interface MyClass + @property(nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setFoo:); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprMultipleMatches) { + testWalk(R"objc( + @interface MyClass1 + $ambiguous^- (void)doSomething; + @end + + @interface MyClass2 + $ambiguous^- (void)doSomething; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doSomething); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprInProtocol) { + testWalk(R"objc( + @protocol MyProtocol + $ambiguous^- (void)protocolMethod; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^protocolMethod); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprMultiColon) { + testWalk(R"objc( + @interface MyClass + $ambiguous^- (void)doA:(int)a withB:(int)b; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doA:withB:); + } + )objc", + {"-x", "objective-c"}); +} + } // namespace } // namespace clang::include_cleaner >From af1df5124fcb965dcb555d8b0db97b826c55aeee Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 31 Jul 2026 14:34:32 -0700 Subject: [PATCH 2/6] Updated with special case for read only properties. Added test --- clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 7 +++++-- .../include-cleaner/unittests/WalkASTTest.cpp | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 03bd3e8f61d00..f88a01dcdc5b5 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -61,8 +61,11 @@ class ObjCSelectorDeclMapBuilder if (Prop) { if (auto Getter = Prop->getGetterName(); !Getter.isNull()) Map[Getter].push_back(Prop); - if (auto Setter = Prop->getSetterName(); !Setter.isNull()) - Map[Setter].push_back(Prop); + if (!Prop->isReadOnly()) { + if (auto Setter = Prop->getSetterName(); !Setter.isNull()) { + Map[Setter].push_back(Prop); + } + } } } } diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index 6dd01f44f1223..bfad8276fa2e6 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -1208,6 +1208,21 @@ TEST(WalkAST, ObjCSelectorExprPropertySetter) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCSelectorExprReadOnlyPropertySetter) { + // Read-only properties do not generate setter selectors. + testWalk(R"objc( + @interface MyClass + @property(readonly, nonatomic) int foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setFoo:); + } + )objc", + {"-x", "objective-c"}); +} + TEST(WalkAST, ObjCSelectorExprMultipleMatches) { testWalk(R"objc( @interface MyClass1 >From 83554689626e96d601e159f8c94ec8011af4c6a5 Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 7 Aug 2026 16:53:52 -0700 Subject: [PATCH 3/6] Cleaned up passing by rvalue ref as it isn't needed. --- clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index f88a01dcdc5b5..254685c8f558a 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -46,7 +46,7 @@ using DeclCallback = class ObjCSelectorDeclMapBuilder : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { public: - ObjCSelectorMap build() && { return std::move(Map); } + ObjCSelectorMap takeMap() { return std::move(Map); } bool TraverseDecl(clang::Decl *D) { if (!D) @@ -627,7 +627,7 @@ ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { if (Ctx.getLangOpts().ObjC) { Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); } - return std::move(Builder).build(); + return Builder.takeMap(); } void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, >From dc564119504de9d80a4b9961b3b1623d20b1d6b0 Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Thu, 13 Aug 2026 11:42:09 -0700 Subject: [PATCH 4/6] Based on comments: - Changed over to a conditional post traversal IFF there are selectors to be resolved. - Resolved issues with both the getter and the property being recorded (and added test) - Added tests for `@property(getter=isFoo, setter=setTheFoo)) int foo` - Cleaned up style nits --- .../include-cleaner/lib/Analysis.cpp | 47 +++++----- .../include-cleaner/lib/AnalysisInternal.h | 13 +-- .../include-cleaner/lib/HTMLReport.cpp | 46 +++++----- .../include-cleaner/lib/WalkAST.cpp | 83 ++++++++++------- .../include-cleaner/unittests/WalkASTTest.cpp | 92 +++++++++++++++---- 5 files changed, 171 insertions(+), 110 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index 5dfa291573723..e48a380211af0 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -56,33 +56,28 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; - ObjCSelectorMap SelectorDecls; - if (!ASTRoots.empty()) { - SelectorDecls = buildObjCSelectorMap(ASTRoots.front()->getASTContext()); - } for (auto *Root : ASTRoots) { - walkAST(*Root, SelectorDecls, - [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch - // space and clang currently doesn't have a good/simple APIs for - // tracking where pieces of a concataned token originated from. So - // we use the macro expansion location instead, and downgrade - // reference type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (RT == RefType::Explicit) - RT = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - // FIXME: Most of the work done here is repetitive. It might be - // useful to have a cache/batching. - SymbolReference SymRef{ND, Loc, RT}; - return CB(SymRef, headersForSymbol(ND, PP, PI)); - }); + walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concataned token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (RT == RefType::Explicit) + RT = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + // FIXME: Most of the work done here is repetitive. It might be useful to + // have a cache/batching. + SymbolReference SymRef{ND, Loc, RT}; + return CB(SymRef, headersForSymbol(ND, PP, PI)); + }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index c9c3042423a56..7d170fd15014d 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -25,12 +25,9 @@ #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" -#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" #include "clang/Lex/Preprocessor.h" -#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" -#include "llvm/ADT/SmallVector.h" #include <vector> namespace clang { @@ -41,14 +38,6 @@ class NamedDecl; class SourceLocation; namespace include_cleaner { -using ObjCSelectorMap = - llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; - -/// Builds a map from Objective-C Selectors to their declarations in the given -/// ASTContext. -/// This is used to optimize selector lookups during AST walking. -ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); - /// Traverses part of the AST from \p Root, finding uses of symbols. /// /// Each use is reported to the callback: @@ -61,7 +50,7 @@ ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); /// /// walkAST is typically called once per top-level declaration in the file /// being analyzed, in order to find all references within it. -void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, +void walkAST(Decl &Root, llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>); /// Finds the headers that provide the symbol location. diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp index c7a8b1728925f..3e067f84432ac 100644 --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -503,31 +503,29 @@ void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, llvm::raw_ostream &OS) { Reporter R(OS, Ctx, PP, Includes, PI, File); const auto &SM = Ctx.getSourceManager(); - ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(Ctx); for (Decl *Root : Roots) - walkAST(*Root, SelectorDecls, - [&](SourceLocation Loc, const NamedDecl &D, RefType T) { - // FIXME: we should merge this logic with `walkUsed` to prevent - // divergences in the future. It isn't trivial though, as we also - // update RefType. Since HTMLReport is only used for debugging - // purposes, divergences aren't critical. - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch - // space and clang currently doesn't have a good/simple APIs for - // tracking where pieces of a concataned token originated from. So - // we use the macro expansion location instead, and downgrade - // reference type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (T == RefType::Explicit) - T = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - R.addRef(SymbolReference{D, Loc, T}); - }); + walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { + // FIXME: we should merge this logic with `walkUsed` to prevent + // divergences in the future. It isn't trivial though, as we also update + // RefType. Since HTMLReport is only used for debugging purposes, + // divergences aren't critical. + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concataned token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (T == RefType::Explicit) + T = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + R.addRef(SymbolReference{D, Loc, T}); + }); for (const SymbolReference &Ref : MacroRefs) { if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) continue; diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 254685c8f558a..770b64b065846 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -27,6 +27,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" @@ -43,28 +44,35 @@ bool isOperatorNewDelete(OverloadedOperatorKind OpKind) { using DeclCallback = llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>; -class ObjCSelectorDeclMapBuilder - : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { +using SelectorMap = llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; + +class TargetedSelectorDeclCollector + : public RecursiveASTVisitor<TargetedSelectorDeclCollector> { public: - ObjCSelectorMap takeMap() { return std::move(Map); } + explicit TargetedSelectorDeclCollector( + const llvm::DenseSet<Selector> &NeededSelectors) + : NeededSelectors(NeededSelectors) {} + + SelectorMap takeMap() { return std::move(Map); } bool TraverseDecl(clang::Decl *D) { if (!D) return true; if (auto *Container = llvm::dyn_cast<clang::ObjCContainerDecl>(D)) { - for (clang::ObjCMethodDecl *M : Container->methods()) { - if (M) { + for (auto *M : Container->methods()) { + if (M && !M->isPropertyAccessor() && + NeededSelectors.contains(M->getSelector())) Map[M->getSelector()].push_back(M); - } } - for (clang::ObjCPropertyDecl *Prop : Container->properties()) { + for (auto *Prop : Container->properties()) { if (Prop) { - if (auto Getter = Prop->getGetterName(); !Getter.isNull()) + if (auto Getter = Prop->getGetterName(); + !Getter.isNull() && NeededSelectors.contains(Getter)) Map[Getter].push_back(Prop); if (!Prop->isReadOnly()) { - if (auto Setter = Prop->getSetterName(); !Setter.isNull()) { + if (auto Setter = Prop->getSetterName(); + !Setter.isNull() && NeededSelectors.contains(Setter)) Map[Setter].push_back(Prop); - } } } } @@ -73,12 +81,13 @@ class ObjCSelectorDeclMapBuilder } private: - ObjCSelectorMap Map; + const llvm::DenseSet<Selector> &NeededSelectors; + SelectorMap Map; }; class ASTWalker : public RecursiveASTVisitor<ASTWalker> { DeclCallback Callback; - const ObjCSelectorMap &SelectorDecls; + llvm::SmallVector<ObjCSelectorExpr *, 4> RecordedSelectors; void report(SourceLocation Loc, NamedDecl *ND, RefType RT = RefType::Explicit) { @@ -140,8 +149,11 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } public: - ASTWalker(DeclCallback Callback, const ObjCSelectorMap &SelectorDecls) - : Callback(Callback), SelectorDecls(SelectorDecls) {} + ASTWalker(DeclCallback Callback) : Callback(Callback) {} + + llvm::ArrayRef<ObjCSelectorExpr *> getRecordedSelectors() const { + return RecordedSelectors; + } // Operators are almost always ADL extension points and by design references // to them doesn't count as uses (generally the type should provide them, so @@ -519,13 +531,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } bool VisitObjCSelectorExpr(ObjCSelectorExpr *E) { - auto Sel = E->getSelector(); - auto It = SelectorDecls.find(Sel); - if (It != SelectorDecls.end()) { - for (NamedDecl *ND : It->second) { - report(E->getSelectorNameLoc(), ND, RefType::Ambiguous); - } - } + RecordedSelectors.push_back(E); return true; } @@ -622,17 +628,32 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } // namespace -ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { - ObjCSelectorDeclMapBuilder Builder; - if (Ctx.getLangOpts().ObjC) { - Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); +void walkAST(Decl &Root, DeclCallback Callback) { + ASTWalker Walker(Callback); + Walker.TraverseDecl(&Root); + + ASTContext &Ctx = Root.getASTContext(); + if (!Ctx.getLangOpts().ObjC) + return; + + auto RecordedSelectors = Walker.getRecordedSelectors(); + if (RecordedSelectors.empty()) + return; + + llvm::DenseSet<Selector> NeededSelectors; + for (const auto *E : RecordedSelectors) + NeededSelectors.insert(E->getSelector()); + TargetedSelectorDeclCollector Collector(NeededSelectors); + Collector.TraverseDecl(Ctx.getTranslationUnitDecl()); + auto Map = Collector.takeMap(); + for (const auto *E : RecordedSelectors) { + auto It = Map.find(E->getSelector()); + if (It != Map.end()) { + for (NamedDecl *ND : It->second) + Callback(E->getSelectorNameLoc(), + *cast<NamedDecl>(ND->getCanonicalDecl()), RefType::Ambiguous); + } } - return Builder.takeMap(); -} - -void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, - DeclCallback Callback) { - ASTWalker(Callback, SelectorDecls).TraverseDecl(&Root); } } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index bfad8276fa2e6..200a21466738b 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -67,20 +67,18 @@ testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode, std::vector<Decl::Kind> TargetDecls; // Perform the walk, and capture the offsets of the referenced targets. std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets; - ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(AST.context()); for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first) continue; - walkAST(*D, SelectorDecls, - [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - if (SM.getFileLoc(Loc) != ReferencingLoc) - return; - auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); - if (NDLoc.first != TargetFile) - return; - ReferencedOffsets[RT].push_back(NDLoc.second); - TargetDecls.push_back(ND.getKind()); - }); + walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + if (SM.getFileLoc(Loc) != ReferencingLoc) + return; + auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); + if (NDLoc.first != TargetFile) + return; + ReferencedOffsets[RT].push_back(NDLoc.second); + TargetDecls.push_back(ND.getKind()); + }); } for (auto &Entry : ReferencedOffsets) llvm::sort(Entry.second); @@ -1181,31 +1179,33 @@ TEST(WalkAST, ObjCSelectorExpr) { } TEST(WalkAST, ObjCSelectorExprPropertyGetter) { - testWalk(R"objc( + auto Decls = testWalk(R"objc( @interface MyClass @property(nonatomic) int $ambiguous^foo; @end )objc", - R"objc( + R"objc( void test() { SEL s = @selector(^foo); } )objc", - {"-x", "objective-c"}); + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); } TEST(WalkAST, ObjCSelectorExprPropertySetter) { - testWalk(R"objc( + auto Decls = testWalk(R"objc( @interface MyClass @property(nonatomic) int $ambiguous^foo; @end )objc", - R"objc( + R"objc( void test() { SEL s = @selector(^setFoo:); } )objc", - {"-x", "objective-c"}); + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); } TEST(WalkAST, ObjCSelectorExprReadOnlyPropertySetter) { @@ -1269,5 +1269,63 @@ TEST(WalkAST, ObjCSelectorExprMultiColon) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCPropertyRefExprCustomGetter) { + testWalk(R"objc( + @interface $implicit^MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $explicit^foo; + @end + )objc", + R"objc( + void test(MyClass *obj) { + int x = obj.^foo; + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCPropertyRefExprCustomSetter) { + testWalk(R"objc( + @interface $implicit^MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $explicit^foo; + @end + )objc", + R"objc( + void test(MyClass *obj) { + obj.^foo = 42; + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprCustomPropertyGetter) { + auto Decls = testWalk(R"objc( + @interface MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^isFoo); + } + )objc", + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); +} + +TEST(WalkAST, ObjCSelectorExprCustomPropertySetter) { + auto Decls = testWalk(R"objc( + @interface MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setTheFoo:); + } + )objc", + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); +} + } // namespace } // namespace clang::include_cleaner >From 42940ca059dbc9da112e3dcd94374b38113b56b9 Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 14 Aug 2026 10:03:58 -0700 Subject: [PATCH 5/6] - Fixed syntactic issues (above and beyond what was reported) - Implemented `bool VisitObjCContainerDecl(ObjCContainerDecl *Container)` instead of `TraverseDecl` --- .../include-cleaner/lib/WalkAST.cpp | 82 +++++++++---------- 1 file changed, 37 insertions(+), 45 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 770b64b065846..ad8a03c9bdffb 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -23,6 +23,7 @@ #include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h" #include "clang/Basic/IdentifierTable.h" +#include "clang/Basic/LLVM.h" #include "clang/Basic/OperatorKinds.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" @@ -30,7 +31,6 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" #include <utility> @@ -44,7 +44,7 @@ bool isOperatorNewDelete(OverloadedOperatorKind OpKind) { using DeclCallback = llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>; -using SelectorMap = llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; +using SelectorMap = llvm::DenseMap<Selector, SmallVector<NamedDecl *, 2>>; class TargetedSelectorDeclCollector : public RecursiveASTVisitor<TargetedSelectorDeclCollector> { @@ -55,29 +55,23 @@ class TargetedSelectorDeclCollector SelectorMap takeMap() { return std::move(Map); } - bool TraverseDecl(clang::Decl *D) { - if (!D) - return true; - if (auto *Container = llvm::dyn_cast<clang::ObjCContainerDecl>(D)) { - for (auto *M : Container->methods()) { - if (M && !M->isPropertyAccessor() && - NeededSelectors.contains(M->getSelector())) - Map[M->getSelector()].push_back(M); - } - for (auto *Prop : Container->properties()) { - if (Prop) { - if (auto Getter = Prop->getGetterName(); - !Getter.isNull() && NeededSelectors.contains(Getter)) - Map[Getter].push_back(Prop); - if (!Prop->isReadOnly()) { - if (auto Setter = Prop->getSetterName(); - !Setter.isNull() && NeededSelectors.contains(Setter)) - Map[Setter].push_back(Prop); - } - } + bool VisitObjCContainerDecl(ObjCContainerDecl *Container) { + for (auto *M : Container->methods()) { + auto Selector = M->getSelector(); + if (!M->isPropertyAccessor() && NeededSelectors.contains(Selector)) + Map[Selector].push_back(M); + } + for (auto *Prop : Container->properties()) { + if (auto Getter = Prop->getGetterName(); + !Getter.isNull() && NeededSelectors.contains(Getter)) + Map[Getter].push_back(Prop); + if (!Prop->isReadOnly()) { + if (auto Setter = Prop->getSetterName(); + !Setter.isNull() && NeededSelectors.contains(Setter)) + Map[Setter].push_back(Prop); } } - return RecursiveASTVisitor::TraverseDecl(D); + return true; } private: @@ -87,7 +81,7 @@ class TargetedSelectorDeclCollector class ASTWalker : public RecursiveASTVisitor<ASTWalker> { DeclCallback Callback; - llvm::SmallVector<ObjCSelectorExpr *, 4> RecordedSelectors; + SmallVector<ObjCSelectorExpr *, 4> RecordedSelectors; void report(SourceLocation Loc, NamedDecl *ND, RefType RT = RefType::Explicit) { @@ -130,7 +124,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // implies we'll point at the using-decl even when there's an explicit // specializaiton using the exported name, but that's rare. auto *ND = resolveTemplateName(TST->getTemplateName()); - if (llvm::isa_and_present<UsingShadowDecl, TypeAliasTemplateDecl>(ND)) + if (isa_and_present<UsingShadowDecl, TypeAliasTemplateDecl>(ND)) return ND; // This is the underlying decl used by TemplateSpecializationType, can be // null when type is dependent or not resolved to a pattern yet. @@ -151,7 +145,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { public: ASTWalker(DeclCallback Callback) : Callback(Callback) {} - llvm::ArrayRef<ObjCSelectorExpr *> getRecordedSelectors() const { + ArrayRef<ObjCSelectorExpr *> getRecordedSelectors() const { return RecordedSelectors; } @@ -164,13 +158,12 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { if (!WalkUpFromCXXOperatorCallExpr(S)) return false; if (auto *CD = S->getCalleeDecl()) { - if (llvm::isa<CXXMethodDecl>(CD)) { + if (isa<CXXMethodDecl>(CD)) { // Treat this as a regular member reference. report(S->getOperatorLoc(), getMemberProvider(S->getArg(0)->getType()), RefType::Implicit); } else { - report(S->getOperatorLoc(), llvm::dyn_cast<NamedDecl>(CD), - RefType::Implicit); + report(S->getOperatorLoc(), dyn_cast<NamedDecl>(CD), RefType::Implicit); } } for (auto *Arg : S->arguments()) @@ -198,13 +191,13 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // Prefer the underlying decl if FoundDecl isn't a shadow decl, e.g: // - For templates, found-decl is always primary template, but we want the // specializaiton itself. - if (!llvm::isa<UsingShadowDecl>(FD)) + if (!isa<UsingShadowDecl>(FD)) FD = DRE->getDecl(); - // For refs to non-meber-like decls, use the found decl. + // For refs to non-member-like decls, use the found decl. // For member-like decls, we should have a reference from the qualifier to // the container decl instead, which is preferred as it'll handle // aliases/exports properly. - if (!FD->isCXXClassMember() && !llvm::isa<EnumConstantDecl>(FD)) { + if (!FD->isCXXClassMember() && !isa<EnumConstantDecl>(FD)) { // Global operator new/delete [] is available implicitly in every // translation unit, even without including any explicit headers. So treat // those as ambigious to not force inclusion in TUs that transitively @@ -222,7 +215,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // // If it's an enum constant, it must be due to prior decl. Report references // to it when qualifier isn't a type. - if (llvm::isa<EnumConstantDecl>(FD) && qualifierIsNamespaceOrNone(DRE)) + if (isa<EnumConstantDecl>(FD) && qualifierIsNamespaceOrNone(DRE)) report(DRE->getLocation(), FD); return true; } @@ -263,13 +256,13 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // Report all (partial) specializations of a class/var template decl. template <typename TemplateDeclType, typename ParitialDeclType> void reportSpecializations(SourceLocation Loc, NamedDecl *ND) { - const auto *TD = llvm::dyn_cast<TemplateDeclType>(ND); + const auto *TD = dyn_cast<TemplateDeclType>(ND); if (!TD) return; for (auto *Spec : TD->specializations()) report(Loc, Spec, RefType::Ambiguous); - llvm::SmallVector<ParitialDeclType *> PartialSpecializations; + SmallVector<ParitialDeclType *> PartialSpecializations; TD->getPartialSpecializations(PartialSpecializations); for (auto *PartialSpec : PartialSpecializations) report(Loc, PartialSpec, RefType::Ambiguous); @@ -281,7 +274,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // transitive dependencies. Hence we only want to report explicit // references for those if they're used. // But for record decls, spelling of the type always refers to primary - // decl non-ambiguously. Hence spelling is already a use. + // decl unambiguously. Hence spelling is already a use. auto IsUsed = TD->isUsed() || TD->isReferenced() || !TD->getAsFunction(); report(UD->getLocation(), TD, IsUsed ? RefType::Explicit : RefType::Ambiguous); @@ -295,7 +288,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { reportSpecializations<VarTemplateDecl, VarTemplatePartialSpecializationDecl>( UD->getLocation(), TD); - if (const auto *FTD = llvm::dyn_cast<FunctionTemplateDecl>(TD)) + if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(TD)) for (auto *Spec : FTD->specializations()) report(UD->getLocation(), Spec, RefType::Ambiguous); } @@ -308,7 +301,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { report(FD->getLocation(), FD); // Explicit specializaiton/instantiations of a function template requires // primary template. - if (clang::isTemplateExplicitInstantiationOrSpecialization( + if (isTemplateExplicitInstantiationOrSpecialization( FD->getTemplateSpecializationKind())) report(FD->getLocation(), FD->getPrimaryTemplate()); return true; @@ -316,7 +309,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { bool VisitVarDecl(VarDecl *VD) { // Ignore the parameter decl itself (its children were handled elsewhere), // as they don't contribute to the main-file #include. - if (llvm::isa<ParmVarDecl>(VD)) + if (isa<ParmVarDecl>(VD)) return true; // Mark declaration from definition as it needs type-checking. if (VD->isThisDeclarationADefinition()) @@ -349,14 +342,14 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // specialized template. Implicit ones are filtered out by RAV. bool VisitClassTemplateSpecializationDecl(ClassTemplateSpecializationDecl *CTSD) { - if (clang::isTemplateExplicitInstantiationOrSpecialization( + if (isTemplateExplicitInstantiationOrSpecialization( CTSD->getTemplateSpecializationKind())) report(CTSD->getLocation(), CTSD->getSpecializedTemplate()->getTemplatedDecl()); return true; } bool VisitVarTemplateSpecializationDecl(VarTemplateSpecializationDecl *VTSD) { - if (clang::isTemplateExplicitInstantiationOrSpecialization( + if (isTemplateExplicitInstantiationOrSpecialization( VTSD->getTemplateSpecializationKind())) report(VTSD->getLocation(), VTSD->getSpecializedTemplate()->getTemplatedDecl()); @@ -379,9 +372,8 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { // outer type-location somewhere, which will trigger an explicit reference // and per IWYS, it's that spelling's responsibility to bring in necessary // declarations. - RefType RT = llvm::isa<RecordDecl>(ND->getDeclContext()) - ? RefType::Implicit - : RefType::Explicit; + RefType RT = isa<RecordDecl>(ND->getDeclContext()) ? RefType::Implicit + : RefType::Explicit; return report(RefLoc, ND, RT); } @@ -494,7 +486,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { return true; } - bool VisitObjCPropertyDecl(clang::ObjCPropertyDecl *PD) { + bool VisitObjCPropertyDecl(ObjCPropertyDecl *PD) { reportType(PD->getLocation(), PD); return true; } >From f1f43a41e9306066f1f9bc7ac56d61d6dade45ed Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 14 Aug 2026 13:09:59 -0700 Subject: [PATCH 6/6] Traverses the translation unit once for a collection of top level decls as opposed to doing it once per decl. --- .../include-cleaner/lib/Analysis.cpp | 45 +++++++++---------- .../include-cleaner/lib/AnalysisInternal.h | 23 +++++----- .../include-cleaner/lib/HTMLReport.cpp | 45 +++++++++---------- .../include-cleaner/lib/WalkAST.cpp | 9 ++-- .../include-cleaner/unittests/WalkASTTest.cpp | 23 +++++----- 5 files changed, 74 insertions(+), 71 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index e48a380211af0..922a690dfc47c 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -33,6 +33,7 @@ #include <cassert> #include <climits> #include <string> +#include <utility> namespace clang::include_cleaner { @@ -56,29 +57,27 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; - for (auto *Root : ASTRoots) { - walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (RT == RefType::Explicit) - RT = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - // FIXME: Most of the work done here is repetitive. It might be useful to - // have a cache/batching. - SymbolReference SymRef{ND, Loc, RT}; - return CB(SymRef, headersForSymbol(ND, PP, PI)); - }); - } + walkAST(ASTRoots, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concatenated token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambiguous to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (RT == RefType::Explicit) + RT = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + // FIXME: Most of the work done here is repetitive. It might be useful to + // have a cache/batching. + SymbolReference SymRef{ND, Loc, RT}; + return CB(SymRef, headersForSymbol(ND, PP, PI)); + }); for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) || diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index 7d170fd15014d..e79c44d7ba09e 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -25,7 +25,9 @@ #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" +#include "clang/Basic/LLVM.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/STLFunctionalExtras.h" #include <vector> @@ -38,36 +40,35 @@ class NamedDecl; class SourceLocation; namespace include_cleaner { -/// Traverses part of the AST from \p Root, finding uses of symbols. +/// Traverses part of the AST from \p Roots, finding uses of symbols. /// /// Each use is reported to the callback: /// - the SourceLocation describes where the symbol was used. This is usually -/// the primary location of the AST node found under Root. +/// the primary location of the AST node found under Roots. /// - the NamedDecl is the symbol referenced. It is canonical, rather than e.g. /// the redecl actually found by lookup. /// - the RefType describes the relation between the SourceLocation and the /// NamedDecl. /// -/// walkAST is typically called once per top-level declaration in the file +/// walkAST is typically passed all top-level declarations in the file /// being analyzed, in order to find all references within it. -void walkAST(Decl &Root, +void walkAST(ArrayRef<Decl *> Roots, llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>); /// Finds the headers that provide the symbol location. -llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc, - const SourceManager &SM, - const PragmaIncludes *PI); +SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc, + const SourceManager &SM, + const PragmaIncludes *PI); /// A set of locations that provides the declaration. std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S, const LangOptions &LO); /// Write an HTML summary of the analysis to the given stream. -void writeHTMLReport(FileID File, const Includes &, - llvm::ArrayRef<Decl *> Roots, - llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx, +void writeHTMLReport(FileID File, const Includes &, ArrayRef<Decl *> Roots, + ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx, const Preprocessor &PP, PragmaIncludes *PI, - llvm::raw_ostream &OS); + raw_ostream &OS); } // namespace include_cleaner } // namespace clang diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp index 3e067f84432ac..5b24008c1b08d 100644 --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -503,29 +503,28 @@ void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, llvm::raw_ostream &OS) { Reporter R(OS, Ctx, PP, Includes, PI, File); const auto &SM = Ctx.getSourceManager(); - for (Decl *Root : Roots) - walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { - // FIXME: we should merge this logic with `walkUsed` to prevent - // divergences in the future. It isn't trivial though, as we also update - // RefType. Since HTMLReport is only used for debugging purposes, - // divergences aren't critical. - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (T == RefType::Explicit) - T = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - R.addRef(SymbolReference{D, Loc, T}); - }); + walkAST(Roots, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { + // FIXME: we should merge this logic with `walkUsed` to prevent + // divergences in the future. It isn't trivial though, as we also update + // RefType. Since HTMLReport is only used for debugging purposes, + // divergences aren't critical. + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concataned token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (T == RefType::Explicit) + T = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + R.addRef(SymbolReference{D, Loc, T}); + }); for (const SymbolReference &Ref : MacroRefs) { if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) continue; diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index ad8a03c9bdffb..0332c13ba0d49 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -620,11 +620,14 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } // namespace -void walkAST(Decl &Root, DeclCallback Callback) { +void walkAST(llvm::ArrayRef<Decl *> Roots, DeclCallback Callback) { + if (Roots.empty()) + return; ASTWalker Walker(Callback); - Walker.TraverseDecl(&Root); + for (auto *Root : Roots) + Walker.TraverseDecl(Root); - ASTContext &Ctx = Root.getASTContext(); + ASTContext &Ctx = Roots.front()->getASTContext(); if (!Ctx.getLangOpts().ObjC) return; diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index 200a21466738b..97dc912a9be0f 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -67,19 +67,20 @@ testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode, std::vector<Decl::Kind> TargetDecls; // Perform the walk, and capture the offsets of the referenced targets. std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets; + llvm::SmallVector<Decl *> TopLevelDecls; for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { - if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first) - continue; - walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - if (SM.getFileLoc(Loc) != ReferencingLoc) - return; - auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); - if (NDLoc.first != TargetFile) - return; - ReferencedOffsets[RT].push_back(NDLoc.second); - TargetDecls.push_back(ND.getKind()); - }); + if (ReferencingFile == SM.getDecomposedExpansionLoc(D->getLocation()).first) + TopLevelDecls.push_back(D); } + walkAST(TopLevelDecls, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + if (SM.getFileLoc(Loc) != ReferencingLoc) + return; + auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); + if (NDLoc.first != TargetFile) + return; + ReferencedOffsets[RT].push_back(NDLoc.second); + TargetDecls.push_back(ND.getKind()); + }); for (auto &Entry : ReferencedOffsets) llvm::sort(Entry.second); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
