This revision was automatically updated to reflect the committed changes.
Closed by commit rG73453e7adecb: [clangd] Avoid expensive checks of buffer
names in IncludeCleaner (authored by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112652/new/
https://reviews.llvm.org/D112652
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
@@ -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.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -49,10 +49,13 @@
ReferencedLocations findReferencedLocations(ParsedAST &AST);
/// Retrieves IDs of all files containing SourceLocations from \p Locs.
+/// 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);
/// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
+/// FileIDs that are not true files (<built-in> etc) are dropped.
llvm::DenseSet<IncludeStructure::HeaderID>
translateToHeaderIDs(const llvm::DenseSet<FileID> &Files,
const IncludeStructure &Includes, const SourceManager &SM);
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
