diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -52,6 +52,7 @@ /// The output only includes things SourceManager sees as files (not macro IDs). /// This can include , etc that are not true files. llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs, + const IncludeStructure &Includes, const SourceManager &SM); /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs). diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -234,20 +234,35 @@ llvm::DenseSet findReferencedFiles(const llvm::DenseSet &Locs, - const SourceManager &SM) { + const IncludeStructure &Includes, const SourceManager &SM) { std::vector Sorted{Locs.begin(), Locs.end()}; llvm::sort(Sorted); // Group by FileID. - ReferencedFiles Result(SM); + ReferencedFiles Files(SM); for (auto It = Sorted.begin(); It < Sorted.end();) { FileID FID = SM.getFileID(*It); - Result.add(FID, *It); + Files.add(FID, *It); // Cheaply skip over all the other locations from the same FileID. // This avoids lots of redundant Loc->File lookups for the same file. do ++It; while (It != Sorted.end() && SM.isInFileID(*It, FID)); } - return std::move(Result.Files); + // If a header is not self-contained, we consider its symbols a logical part + // of the including file. Therefore, mark the parents of all used + // non-self-contained FileIDs as used. Perform this on FileIDs rather than + // HeaderIDs, as each inclusion of a non-self-contained file is distinct. + llvm::DenseSet Result; + for (FileID ID : Files.Files) { + // Unroll the chain of non self-contained headers to get to the first one + // with the header guard. + for (const FileEntry *FE = SM.getFileEntryForID(ID); + ID != SM.getMainFileID() && FE && + !Includes.isSelfContained(*Includes.getID(FE)); + ID = SM.getFileID(SM.getIncludeLoc(ID)), FE = SM.getFileEntryForID(ID)) + ; + Result.insert(ID); + } + return Result; } std::vector @@ -304,12 +319,8 @@ const auto &SM = AST.getSourceManager(); auto Refs = findReferencedLocations(AST); - // FIXME(kirillbobyrev): Attribute the symbols from non self-contained - // headers to their parents while the information about respective - // SourceLocations and FileIDs is not lost. It is not possible to do that - // later when non-guarded headers included multiple times will get same - // HeaderID. - auto ReferencedFileIDs = findReferencedFiles(Refs, SM); + auto ReferencedFileIDs = findReferencedFiles(Refs, AST.getIncludeStructure(), + AST.getSourceManager()); auto ReferencedHeaders = translateToHeaderIDs(ReferencedFileIDs, AST.getIncludeStructure(), SM); return getUnused(AST, ReferencedHeaders); diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -281,7 +281,8 @@ auto &SM = AST.getSourceManager(); auto &Includes = AST.getIncludeStructure(); - auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM); + auto ReferencedFiles = + findReferencedFiles(findReferencedLocations(AST), Includes, SM); llvm::StringSet<> ReferencedFileNames; for (FileID FID : ReferencedFiles) ReferencedFileNames.insert( @@ -303,6 +304,52 @@ EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty()); } +TEST(IncludeCleaner, NonSelfContainedHeaders) { + TestTU TU; + TU.Code = R"cpp( + #include "bar.h" + #include "foo.h" + + int LocalFoo = foo::Variable, + LocalBar = bar::Variable; + )cpp"; + TU.AdditionalFiles["bar.h"] = R"cpp( + #pragma once + namespace bar { + #include "indirection.h" + } + )cpp"; + TU.AdditionalFiles["foo.h"] = R"cpp( + #ifndef FOO_H + #define FOO_H + namespace foo { + #include "unguarded.h" + } + #endif // FOO_H + )cpp"; + TU.AdditionalFiles["indirection.h"] = R"cpp( + #include "unguarded.h" + )cpp"; + TU.AdditionalFiles["unguarded.h"] = R"cpp( + constexpr int Variable = 42; + )cpp"; + + ParsedAST AST = TU.build(); + + auto ReferencedFiles = + findReferencedFiles(findReferencedLocations(AST), + AST.getIncludeStructure(), AST.getSourceManager()); + llvm::StringSet<> ReferencedFileNames; + auto &SM = AST.getSourceManager(); + for (FileID FID : ReferencedFiles) + ReferencedFileNames.insert( + SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename()); + // Note that we have uplifted the referenced files from non self-contained + // headers to header-guarded ones. + EXPECT_THAT(ReferencedFileNames.keys(), + UnorderedElementsAre(testPath("bar.h"), testPath("foo.h"))); +} + } // namespace } // namespace clangd } // namespace clang