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,37 +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/DenseMap.h" #include "llvm/ADT/SmallVector.h" namespace clang::include_cleaner { +namespace { +// Gets all the providers for a symbol by tarversing each location. +llvm::SmallVector
headersForSymbol(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. + llvm::DenseMap> 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 [It, Inserted] = DeclToHeaders.try_emplace(&ND); + if (Inserted) + It->second = headersForSymbol(ND, SM, PI); + return CB(SymRef, It->second); }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); - CB(MacroRef, findHeaders(MacroRef.Target.macro().Definition, SM, PI)); + return CB(MacroRef, headersForSymbol(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 @@ -154,7 +154,7 @@ }; std::vector Refs; llvm::DenseMap> IncludeRefs; - llvm::StringMap> Insertion; + llvm::StringMap> Insertion; llvm::StringRef includeType(const Include *I) { auto &List = IncludeRefs[I]; @@ -185,17 +185,8 @@ void fillTarget(Ref &R) { // 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 (R.Sym.kind()) { - case Symbol::Declaration: - R.Locations.push_back(R.Sym.declaration().getLocation()); - break; - case Symbol::Macro: - R.Locations.push_back(R.Sym.macro().Definition); - break; - } - + for (auto &Loc : locateSymbol(R.Sym)) + R.Locations.push_back(Loc); for (const auto &Loc : R.Locations) R.Headers.append(findHeaders(Loc, SM, PI)); @@ -220,8 +211,8 @@ public: Reporter(llvm::raw_ostream &OS, ASTContext &Ctx, HeaderSearch &HS, - const include_cleaner::Includes &Includes, - const PragmaIncludes *PI, FileID MainFile) + const include_cleaner::Includes &Includes, const PragmaIncludes *PI, + FileID MainFile) : OS(OS), Ctx(Ctx), SM(Ctx.getSourceManager()), HS(HS), Includes(Includes), PI(PI), MainFile(MainFile), MainFE(SM.getFileEntryForID(MainFile)) {} @@ -321,7 +312,7 @@ printFilename(SM.getSpellingLoc(Loc).printToString(SM)); OS << ">"; } - + // Write "Provides: " rows of an include or include-insertion table. // These describe the symbols the header provides, referenced by RefIndices. void writeProvides(llvm::ArrayRef RefIndices) { @@ -366,7 +357,7 @@ } OS << ""; } - + void writeInsertion(llvm::StringRef Text, llvm::ArrayRef Refs) { OS << ""; writeProvides(Refs); @@ -440,7 +431,7 @@ llvm::sort(Insertions); for (llvm::StringRef Insertion : Insertions) { OS << "" - << "#include "; escapeString(Insertion); 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 @@ -18,6 +18,7 @@ #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/Support/Error.h" #include "llvm/Testing/Support/Annotations.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -25,6 +26,7 @@ namespace clang::include_cleaner { namespace { +using testing::Contains; using testing::Pair; using testing::UnorderedElementsAre; @@ -32,8 +34,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::ArrayRef MacroRefs = {}) { + 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 +82,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,87 +92,76 @@ 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(WalkUsed, MacroRefs) { - llvm::Annotations Hdr(R"cpp( - #define ^ANSWER 42 +TEST_F(WalkUsedTest, MultipleProviders) { + llvm::Annotations Code(R"cpp( + #include "header1.h" + #include "header2.h" + void foo(); + + void bar() { + $foo^foo(); + } )cpp"); - llvm::Annotations Main(R"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_F(WalkUsedTest, MacroRefs) { + llvm::Annotations Code(R"cpp( #include "hdr.h" int x = ^ANSWER; )cpp"); - - SourceManagerForFile SMF("main.cpp", Main.code()); - auto &SM = SMF.get(); - const FileEntry *HdrFile = - SM.getFileManager().getVirtualFile("hdr.h", Hdr.code().size(), 0); - SM.overrideFileContents(HdrFile, - llvm::MemoryBuffer::getMemBuffer(Hdr.code().str())); - FileID HdrID = SM.createFileID(HdrFile, SourceLocation(), SrcMgr::C_User); + llvm::Annotations Hdr(guard("#define ^ANSWER 42")); + Inputs.Code = Code.code(); + Inputs.ExtraFiles["hdr.h"] = Hdr.code(); + TestAST AST(Inputs); + auto &SM = AST.sourceManager(); + auto HdrFile = SM.getFileManager().getFile("hdr.h").get(); + auto HdrID = SM.translateFile(HdrFile); IdentifierTable Idents; Symbol Answer = Macro{&Idents.get("ANSWER"), SM.getComposedLoc(HdrID, Hdr.point())}; - llvm::DenseMap> OffsetToProviders; - walkUsed(/*ASTRoots=*/{}, /*MacroRefs=*/ - {SymbolReference{SM.getComposedLoc(SM.getMainFileID(), Main.point()), - Answer, RefType::Explicit}}, - /*PI=*/nullptr, 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()); - }); - EXPECT_THAT( - OffsetToProviders, - UnorderedElementsAre(Pair(Main.point(), UnorderedElementsAre(HdrFile)))); + offsetToProviders( + AST, SM, + {SymbolReference{SM.getComposedLoc(SM.getMainFileID(), Code.point()), + Answer, RefType::Explicit}}), + UnorderedElementsAre(Pair(Code.point(), UnorderedElementsAre(HdrFile)))); } - } // namespace } // namespace clang::include_cleaner