diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -8,38 +8,56 @@ #include "clang-include-cleaner/Analysis.h" #include "AnalysisInternal.h" +#include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" #include "clang/Basic/SourceManager.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" +#include namespace clang::include_cleaner { +namespace { +// Gets all the providers for a symbol by tarversing each location. +llvm::SmallVector
findAllHeaders(const Symbol &S, + const SourceManager &SM, + const PragmaIncludes *PI) { + llvm::SmallVector
Headers; + for (auto &Loc : locateSymbol(S)) + // FIXME: findHeaders in theory returns the same result for all source + // locations in the same FileID. Have a cache for that, since we can have + // multiple declarations coming from the same file. + Headers.append(findHeaders(Loc, SM, PI)); + return Headers; +} +} // namespace + void walkUsed(llvm::ArrayRef ASTRoots, llvm::ArrayRef MacroRefs, const PragmaIncludes *PI, const SourceManager &SM, UsedSymbolCB CB) { // This is duplicated in writeHTMLReport, changes should be mirrored there. + + // Cache for decl to header mappings, as the same decl might be referenced in + // multiple locations and finding providers for each location is expensive. + std::unordered_map> DeclToHeaders; tooling::stdlib::Recognizer Recognizer; for (auto *Root : ASTRoots) { auto &SM = Root->getASTContext().getSourceManager(); walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { SymbolReference SymRef{Loc, ND, RT}; - if (auto SS = Recognizer(&ND)) { - // FIXME: Also report forward decls from main-file, so that the caller - // can decide to insert/ignore a header. - return CB(SymRef, findHeaders(*SS, SM, PI)); - } - // FIXME: Extract locations from redecls. - return CB(SymRef, findHeaders(ND.getLocation(), SM, PI)); + auto Inserted = DeclToHeaders.try_emplace(&ND); + if (Inserted.second) + Inserted.first->second = findAllHeaders(ND, SM, PI); + return CB(SymRef, Inserted.first->second); }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); - return CB(MacroRef, - findHeaders(MacroRef.Target.macro().Definition, SM, PI)); + return CB(MacroRef, findAllHeaders(MacroRef.Target, SM, PI)); } } diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -137,22 +137,10 @@ Target makeTarget(const SymbolReference &SR) { Target T{SR.Target, SR.RT, {}, {}}; - - // Duplicates logic from walkUsed(), which doesn't expose SymbolLocations. - // FIXME: use locateDecl and friends once implemented. - // This doesn't use stdlib::Recognizer, but locateDecl will soon do that. - switch (SR.Target.kind()) { - case Symbol::Declaration: - T.Locations.push_back(SR.Target.declaration().getLocation()); - break; - case Symbol::Macro: - T.Locations.push_back(SR.Target.macro().Definition); - break; - } - + for (auto &Loc : locateSymbol(SR.Target)) + T.Locations.push_back(Loc); for (const auto &Loc : T.Locations) - T.Headers = findHeaders(Loc, SM, PI); - + T.Headers.append(findHeaders(Loc, SM, PI)); return T; } diff --git a/clang-tools-extra/include-cleaner/test/Inputs/foo2.h b/clang-tools-extra/include-cleaner/test/Inputs/foo2.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/include-cleaner/test/Inputs/foo2.h @@ -0,0 +1,6 @@ +#ifndef FOO2_H +#define FOO2_H + +int foo(); + +#endif diff --git a/clang-tools-extra/include-cleaner/test/html.cpp b/clang-tools-extra/include-cleaner/test/html.cpp --- a/clang-tools-extra/include-cleaner/test/html.cpp +++ b/clang-tools-extra/include-cleaner/test/html.cpp @@ -1,6 +1,11 @@ // RUN: clang-include-cleaner -html=- %s -- -I %S/Inputs | FileCheck %s #include "bar.h" #include "foo.h" +#include "foo2.h" int n = foo(); +// CHECK: Symbol{{.*}}foo +// CHECK-NEXT: int foo() +// CHECK-NEXT: Location{{.*}}foo.h:4:5 +// CHECK-NEXT: Location{{.*}}foo2.h:4:5 // CHECK: foo() diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -25,6 +25,7 @@ namespace clang::include_cleaner { namespace { +using testing::Contains; using testing::Pair; using testing::UnorderedElementsAre; @@ -32,8 +33,45 @@ return "#pragma once\n" + Code.str(); } -TEST(WalkUsed, Basic) { - // FIXME: Have a fixture for setting up tests. +class WalkUsedTest : public testing::Test { +protected: + TestInputs Inputs; + PragmaIncludes PI; + WalkUsedTest() { + Inputs.MakeAction = [this] { + struct Hook : public SyntaxOnlyAction { + public: + Hook(PragmaIncludes *Out) : Out(Out) {} + bool BeginSourceFileAction(clang::CompilerInstance &CI) override { + Out->record(CI); + return true; + } + + PragmaIncludes *Out; + }; + return std::make_unique(&PI); + }; + } + + llvm::DenseMap> + offsetToProviders(TestAST &AST, SourceManager &SM) { + llvm::SmallVector TopLevelDecls; + for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { + TopLevelDecls.emplace_back(D); + } + llvm::DenseMap> OffsetToProviders; + walkUsed(TopLevelDecls, /*MacroRefs=*/{}, &PI, SM, + [&](const SymbolReference &Ref, llvm::ArrayRef
Providers) { + auto [FID, Offset] = SM.getDecomposedLoc(Ref.RefLocation); + if (FID != SM.getMainFileID()) + ADD_FAILURE() << "Reference outside of the main file!"; + OffsetToProviders.try_emplace(Offset, Providers.vec()); + }); + return OffsetToProviders; + } +}; + +TEST_F(WalkUsedTest, Basic) { llvm::Annotations Code(R"cpp( #include "header.h" #include "private.h" @@ -43,7 +81,7 @@ std::$vector^vector $vconstructor^v; } )cpp"); - TestInputs Inputs(Code.code()); + Inputs.Code = Code.code(); Inputs.ExtraFiles["header.h"] = guard(R"cpp( void foo(); namespace std { class vector {}; } @@ -53,51 +91,53 @@ class Private {}; )cpp"); - PragmaIncludes PI; - Inputs.MakeAction = [&PI] { - struct Hook : public SyntaxOnlyAction { - public: - Hook(PragmaIncludes *Out) : Out(Out) {} - bool BeginSourceFileAction(clang::CompilerInstance &CI) override { - Out->record(CI); - return true; - } - - PragmaIncludes *Out; - }; - return std::make_unique(&PI); - }; TestAST AST(Inputs); - - llvm::SmallVector TopLevelDecls; - for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { - TopLevelDecls.emplace_back(D); - } - auto &SM = AST.sourceManager(); - llvm::DenseMap> OffsetToProviders; - walkUsed(TopLevelDecls, /*MacroRefs=*/{}, &PI, SM, - [&](const SymbolReference &Ref, llvm::ArrayRef
Providers) { - auto [FID, Offset] = SM.getDecomposedLoc(Ref.RefLocation); - EXPECT_EQ(FID, SM.getMainFileID()); - OffsetToProviders.try_emplace(Offset, Providers.vec()); - }); - auto &FM = AST.fileManager(); - auto HeaderFile = Header(FM.getFile("header.h").get()); + auto HeaderFile = Header(AST.fileManager().getFile("header.h").get()); + auto PrivateFile = Header(AST.fileManager().getFile("private.h").get()); + auto PublicFile = Header("\"path/public.h\""); auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID())); auto VectorSTL = Header(tooling::stdlib::Header::named("").value()); EXPECT_THAT( - OffsetToProviders, + offsetToProviders(AST, SM), UnorderedElementsAre( Pair(Code.point("bar"), UnorderedElementsAre(MainFile)), Pair(Code.point("private"), - UnorderedElementsAre(Header("\"path/public.h\""), - Header(FM.getFile("private.h").get()))), + UnorderedElementsAre(PublicFile, PrivateFile)), Pair(Code.point("foo"), UnorderedElementsAre(HeaderFile)), Pair(Code.point("vector"), UnorderedElementsAre(VectorSTL)), Pair(Code.point("vconstructor"), UnorderedElementsAre(VectorSTL)))); } +TEST_F(WalkUsedTest, MultipleProviders) { + llvm::Annotations Code(R"cpp( + #include "header1.h" + #include "header2.h" + void foo(); + + void bar() { + $foo^foo(); + } + )cpp"); + Inputs.Code = Code.code(); + Inputs.ExtraFiles["header1.h"] = guard(R"cpp( + void foo(); + )cpp"); + Inputs.ExtraFiles["header2.h"] = guard(R"cpp( + void foo(); + )cpp"); + + TestAST AST(Inputs); + auto &SM = AST.sourceManager(); + auto HeaderFile1 = Header(AST.fileManager().getFile("header1.h").get()); + auto HeaderFile2 = Header(AST.fileManager().getFile("header2.h").get()); + auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID())); + EXPECT_THAT( + offsetToProviders(AST, SM), + Contains(Pair(Code.point("foo"), + UnorderedElementsAre(HeaderFile1, HeaderFile2, MainFile)))); +} + TEST(WalkUsed, MacroRefs) { llvm::Annotations Hdr(R"cpp( #define ^ANSWER 42 @@ -134,6 +174,5 @@ UnorderedElementsAre(Pair(Main.point(), UnorderedElementsAre(HdrFile)))); } - } // namespace } // namespace clang::include_cleaner