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,7 +52,8 @@ /// 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 SourceManager &SM); + const SourceManager &SM, + const Preprocessor &PP); /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs). /// FileIDs that are not true files ( etc) are dropped. 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 @@ -16,6 +16,7 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Syntax/Tokens.h" @@ -137,23 +138,49 @@ llvm::DenseSet Macros; const SourceManager &SM; - void add(SourceLocation Loc) { add(SM.getFileID(Loc), Loc); } + // Attributes the locations 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. + // + // Returns true if the insertion into IDs took place. + bool insert(llvm::DenseSet &IDs, FileID ID, const SourceManager &SM, + const Preprocessor &PP) { + if (IDs.contains(ID)) + return false; + // 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 && + !PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);) { + ID = SM.getFileID(SM.getIncludeLoc(ID)); + FE = SM.getFileEntryForID(ID); + } + return IDs.insert(ID).second; + } + + void add(SourceLocation Loc, const SourceManager &SM, + const Preprocessor &PP) { + + add(SM.getFileID(Loc), Loc, SM, PP); + } - void add(FileID FID, SourceLocation Loc) { + void add(FileID FID, SourceLocation Loc, const SourceManager &SM, + const Preprocessor &PP) { if (FID.isInvalid()) return; assert(SM.isInFileID(Loc, FID)); if (Loc.isFileID()) { - Files.insert(FID); + insert(Files, FID, SM, PP); return; } // Don't process the same macro FID twice. - if (!Macros.insert(FID).second) + if (!insert(Macros, FID, SM, PP)) return; const auto &Exp = SM.getSLocEntry(FID).getExpansion(); - add(Exp.getSpellingLoc()); - add(Exp.getExpansionLocStart()); - add(Exp.getExpansionLocEnd()); + add(Exp.getSpellingLoc(), SM, PP); + add(Exp.getExpansionLocStart(), SM, PP); + add(Exp.getExpansionLocEnd(), SM, PP); } }; @@ -234,13 +261,13 @@ llvm::DenseSet findReferencedFiles(const llvm::DenseSet &Locs, - const SourceManager &SM) { + const SourceManager &SM, const Preprocessor &PP) { std::vector Sorted{Locs.begin(), Locs.end()}; llvm::sort(Sorted); // Group by FileID. ReferencedFiles Result(SM); for (auto It = Sorted.begin(); It < Sorted.end();) { FileID FID = SM.getFileID(*It); - Result.add(FID, *It); + Result.add(FID, *It, SM, PP); // Cheaply skip over all the other locations from the same FileID. // This avoids lots of redundant Loc->File lookups for the same file. do @@ -304,12 +331,7 @@ 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, SM, AST.getPreprocessor()); 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), SM, + AST.getPreprocessor()); llvm::StringSet<> ReferencedFileNames; for (FileID FID : ReferencedFiles) ReferencedFileNames.insert( @@ -303,6 +304,60 @@ 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( + #pragma once + namespace foo { + #include "unguarded.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 &SM = AST.getSourceManager(); + auto &Includes = AST.getIncludeStructure(); + + auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM, + AST.getPreprocessor()); + llvm::StringSet<> ReferencedFileNames; + 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"))); + + auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM); + std::vector ReferencedHeaderNames; + for (IncludeStructure::HeaderID HID : ReferencedHeaders) + ReferencedHeaderNames.push_back(Includes.getRealPath(HID)); + EXPECT_THAT(ReferencedHeaderNames, + ElementsAre(testPath("bar.h"), testPath("foo.h"))); + + // Sanity check. + EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang