kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
When a symbol comes from the non self-contained header, we recursively uplift
the file we consider used to the first includer that has a header guard. We
need to do this while we still have FileIDs because every time a non
self-contained header is included, it gets a new FileID but is later
deduplicated by HeaderID and it's not possible to understand where it was
included from.
Based on D114370 <https://reviews.llvm.org/D114370>.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114623
Files:
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ 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
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ 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 <built-in>, <scratch space> etc that are not true files.
llvm::DenseSet<FileID> findReferencedFiles(const ReferencedLocations &Locs,
+ const IncludeStructure &Includes,
const SourceManager &SM);
/// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -234,20 +234,35 @@
llvm::DenseSet<FileID>
findReferencedFiles(const llvm::DenseSet<SourceLocation> &Locs,
- const SourceManager &SM) {
+ const IncludeStructure &Includes, const SourceManager &SM) {
std::vector<SourceLocation> 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<FileID> 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<const Inclusion *>
@@ -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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
