kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: usaxena95, 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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114370
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), 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<llvm::StringRef> 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
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ 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 <built-in>, <scratch space> etc that are not true files.
llvm::DenseSet<FileID> 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 (<built-in> etc) are dropped.
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ 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<FileID> 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<FileID> &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<FileID>
findReferencedFiles(const llvm::DenseSet<SourceLocation> &Locs,
- const SourceManager &SM) {
+ const SourceManager &SM, const Preprocessor &PP) {
std::vector<SourceLocation> 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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
