sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
This changes the handling of special buffers (<command-line> etc) that
SourceManager treats as files but FileManager does not.
We now include them in findReferencedFiles() and drop them as part of
translateToHeaderIDs(). This pairs more naturally with the data representations
we're using, and so avoids a bunch of converting between representations for
filtering.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D112652
Files:
clang-tools-extra/clangd/IncludeCleaner.cpp
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
@@ -16,6 +16,7 @@
namespace clangd {
namespace {
+using ::testing::ElementsAre;
using ::testing::UnorderedElementsAre;
TEST(IncludeCleaner, ReferencedLocations) {
@@ -236,9 +237,8 @@
TEST(IncludeCleaner, VirtualBuffers) {
TestTU TU;
- TU.Filename = "foo.cpp";
TU.Code = R"cpp(
- #include "macro_spelling_in_scratch_buffer.h"
+ #include "macros.h"
using flags::FLAGS_FOO;
@@ -251,7 +251,7 @@
// The pasting operator in combination with DEFINE_FLAG will create
// ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not
// FileEntry.
- TU.AdditionalFiles["macro_spelling_in_scratch_buffer.h"] = R"cpp(
+ TU.AdditionalFiles["macros.h"] = R"cpp(
#define DEFINE_FLAG(X) \
namespace flags { \
int FLAGS_##X; \
@@ -266,18 +266,27 @@
ParsedAST AST = TU.build();
auto &SM = AST.getSourceManager();
auto &Includes = AST.getIncludeStructure();
+
auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
- auto Entry = SM.getFileManager().getFile(
- testPath("macro_spelling_in_scratch_buffer.h"));
- ASSERT_TRUE(Entry);
- auto FID = SM.translateFile(*Entry);
- // No "<scratch space>" FID.
- EXPECT_THAT(ReferencedFiles, UnorderedElementsAre(FID));
- // Should not crash due to <scratch space> "files" missing from include
- // structure.
- EXPECT_THAT(
- getUnused(Includes, translateToHeaderIDs(ReferencedFiles, Includes, SM)),
- ::testing::IsEmpty());
+ llvm::StringSet<> ReferencedFileNames;
+ for (FileID FID : ReferencedFiles)
+ ReferencedFileNames.insert(
+ SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+ // (Note we deduped the names as _number_ of <built-in>s is uninteresting).
+ EXPECT_THAT(ReferencedFileNames.keys(),
+ UnorderedElementsAre("<built-in>", "<scratch space>",
+ testPath("macros.h")));
+
+ // Should not crash due to FileIDs that are not headers.
+ auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM);
+ std::vector<llvm::StringRef> ReferencedHeaderNames;
+ for (IncludeStructure::HeaderID HID : ReferencedHeaders)
+ ReferencedHeaderNames.push_back(Includes.getRealPath(HID));
+ // Non-header files are gone at this point.
+ EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
+
+ // Sanity check.
+ EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
}
} // namespace
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -129,9 +129,7 @@
void add(SourceLocation Loc) { add(SM.getFileID(Loc), Loc); }
void add(FileID FID, SourceLocation Loc) {
- // Check if Loc is not written in a physical file.
- if (FID.isInvalid() || SM.isWrittenInBuiltinFile(Loc) ||
- SM.isWrittenInCommandLineFile(Loc))
+ if (FID.isInvalid())
return;
assert(SM.isInFileID(Loc, FID));
if (Loc.isFileID()) {
@@ -142,15 +140,9 @@
if (!Macros.insert(FID).second)
return;
const auto &Exp = SM.getSLocEntry(FID).getExpansion();
- // For token pasting operator in macros, spelling and expansion locations
- // can be within a temporary buffer that Clang creates (scratch space or
- // ScratchBuffer). That is not a real file we can include.
- if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
- add(Exp.getSpellingLoc());
- if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocStart()))
- add(Exp.getExpansionLocStart());
- if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocEnd()))
- add(Exp.getExpansionLocEnd());
+ add(Exp.getSpellingLoc());
+ add(Exp.getExpansionLocStart());
+ add(Exp.getExpansionLocEnd());
}
};
@@ -249,6 +241,14 @@
return Unused;
}
+#ifndef NDEBUG
+// Is FID a <built-in>, <scratch space> etc?
+static bool isSpecialBuffer(FileID FID, const SourceManager &SM) {
+ const SrcMgr::FileInfo &FI = SM.getSLocEntry(FID).getFile();
+ return FI.getName().startswith("<");
+}
+#endif
+
llvm::DenseSet<IncludeStructure::HeaderID>
translateToHeaderIDs(const llvm::DenseSet<FileID> &Files,
const IncludeStructure &Includes,
@@ -257,7 +257,10 @@
TranslatedHeaderIDs.reserve(Files.size());
for (FileID FID : Files) {
const FileEntry *FE = SM.getFileEntryForID(FID);
- assert(FE);
+ if (!FE) {
+ assert(isSpecialBuffer(FID, SM));
+ continue;
+ }
const auto File = Includes.getID(FE);
assert(File);
TranslatedHeaderIDs.insert(*File);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
