kadircet updated this revision to Diff 480419.
kadircet marked an inline comment as done.
kadircet added a comment.
- Drop the cache
- Use `-print=changes` in lit test
- Update unittest helper to limit decls to main file
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138200/new/
https://reviews.llvm.org/D138200
Files:
clang-tools-extra/include-cleaner/lib/Analysis.cpp
clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
clang-tools-extra/include-cleaner/test/Inputs/foo2.h
clang-tools-extra/include-cleaner/test/multiple-providers.cpp
clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
===================================================================
--- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -18,7 +18,6 @@
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
-#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Testing/Support/Annotations.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -26,6 +25,7 @@
namespace clang::include_cleaner {
namespace {
+using testing::Contains;
using testing::ElementsAre;
using testing::Pair;
using testing::UnorderedElementsAre;
@@ -34,8 +34,48 @@
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<Hook>(&PI);
+ };
+ }
+
+ llvm::DenseMap<size_t, std::vector<Header>>
+ offsetToProviders(TestAST &AST, SourceManager &SM,
+ llvm::ArrayRef<SymbolReference> MacroRefs = {}) {
+ llvm::SmallVector<Decl *> TopLevelDecls;
+ for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) {
+ if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation())))
+ continue;
+ TopLevelDecls.emplace_back(D);
+ }
+ llvm::DenseMap<size_t, std::vector<Header>> OffsetToProviders;
+ walkUsed(TopLevelDecls, MacroRefs, &PI, SM,
+ [&](const SymbolReference &Ref, llvm::ArrayRef<Header> 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"
@@ -45,7 +85,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 {}; }
@@ -55,85 +95,75 @@
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<Hook>(&PI);
- };
TestAST AST(Inputs);
-
- llvm::SmallVector<Decl *> TopLevelDecls;
- for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) {
- TopLevelDecls.emplace_back(D);
- }
-
auto &SM = AST.sourceManager();
- llvm::DenseMap<size_t, std::vector<Header>> OffsetToProviders;
- walkUsed(TopLevelDecls, /*MacroRefs=*/{}, &PI, SM,
- [&](const SymbolReference &Ref, llvm::ArrayRef<Header> 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("<vector>").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");
+ Inputs.Code = Code.code();
+ Inputs.ExtraFiles["header1.h"] = guard(R"cpp(
+ void foo();
+ )cpp");
+ Inputs.ExtraFiles["header2.h"] = guard(R"cpp(
+ void foo();
)cpp");
- llvm::Annotations Main(R"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<size_t, std::vector<Header>> OffsetToProviders;
- walkUsed(/*ASTRoots=*/{}, /*MacroRefs=*/
- {SymbolReference{SM.getComposedLoc(SM.getMainFileID(), Main.point()),
- Answer, RefType::Explicit}},
- /*PI=*/nullptr, SM,
- [&](const SymbolReference &Ref, llvm::ArrayRef<Header> 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))));
}
TEST(Analyze, Basic) {
Index: clang-tools-extra/include-cleaner/test/multiple-providers.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/include-cleaner/test/multiple-providers.cpp
@@ -0,0 +1,9 @@
+// RUN: clang-include-cleaner --print=changes %s -- -I %S/Inputs | FileCheck \
+// --allow-empty %s
+#include "foo.h"
+#include "foo2.h"
+
+int n = foo();
+// Make sure both providers are preserved.
+// CHECK-NOT: - "foo.h"
+// CHECK-NOT: - "foo2.h"
Index: clang-tools-extra/include-cleaner/test/Inputs/foo2.h
===================================================================
--- /dev/null
+++ clang-tools-extra/include-cleaner/test/Inputs/foo2.h
@@ -0,0 +1,6 @@
+#ifndef FOO2_H
+#define FOO2_H
+
+int foo();
+
+#endif
Index: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
+++ clang-tools-extra/include-cleaner/lib/HTMLReport.cpp
@@ -154,7 +154,7 @@
};
std::vector<Ref> Refs;
llvm::DenseMap<const Include *, std::vector<unsigned>> IncludeRefs;
- llvm::StringMap<std::vector</*RefIndex*/unsigned>> Insertion;
+ llvm::StringMap<std::vector</*RefIndex*/ unsigned>> 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<unsigned> RefIndices) {
@@ -366,7 +357,7 @@
}
OS << "</table>";
}
-
+
void writeInsertion(llvm::StringRef Text, llvm::ArrayRef<unsigned> Refs) {
OS << "<table class='insertion'>";
writeProvides(Refs);
@@ -440,7 +431,7 @@
llvm::sort(Insertions);
for (llvm::StringRef Insertion : Insertions) {
OS << "<code class='line added'>"
- << "<span class='inc sel inserted' data-hover='i";
+ << "<span class='inc sel inserted' data-hover='i";
escapeString(Insertion);
OS << "'>#include ";
escapeString(Insertion);
Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp
===================================================================
--- clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -8,8 +8,10 @@
#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/Format/Format.h"
#include "clang/Lex/HeaderSearch.h"
@@ -21,6 +23,21 @@
namespace clang::include_cleaner {
+namespace {
+// Gets all the providers for a symbol by tarversing each location.
+llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
+ const SourceManager &SM,
+ const PragmaIncludes *PI) {
+ llvm::SmallVector<Header> 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<Decl *> ASTRoots,
llvm::ArrayRef<SymbolReference> MacroRefs,
const PragmaIncludes *PI, const SourceManager &SM,
@@ -31,18 +48,12 @@
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));
+ return CB(SymRef, headersForSymbol(ND, SM, PI));
});
}
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));
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
