[clang-tools-extra] 73453e7 - [clangd] Avoid expensive checks of buffer names in IncludeCleaner
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 27 23:01:18 PDT 2021
Author: Sam McCall
Date: 2021-10-28T08:00:57+02:00
New Revision: 73453e7adecbcc7208ceb70d5e55086ffbd1de3a
URL: https://github.com/llvm/llvm-project/commit/73453e7adecbcc7208ceb70d5e55086ffbd1de3a
DIFF: https://github.com/llvm/llvm-project/commit/73453e7adecbcc7208ceb70d5e55086ffbd1de3a.diff
LOG: [clangd] Avoid expensive checks of buffer names in IncludeCleaner
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.
Differential Revision: https://reviews.llvm.org/D112652
Added:
Modified:
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 156129be22fee..b813c3f2c0417 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -129,9 +129,7 @@ struct ReferencedFiles {
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 @@ struct ReferencedFiles {
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 @@ getUnused(const IncludeStructure &Structure,
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 @@ translateToHeaderIDs(const llvm::DenseSet<FileID> &Files,
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);
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index c7dd7efd4ed6f..b797e3c8d6f40 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -49,10 +49,13 @@ using ReferencedLocations = llvm::DenseSet<SourceLocation>;
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);
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 99749692c541c..895a2074ad2e7 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -16,6 +16,7 @@ namespace clang {
namespace clangd {
namespace {
+using ::testing::ElementsAre;
using ::testing::UnorderedElementsAre;
TEST(IncludeCleaner, ReferencedLocations) {
@@ -236,9 +237,8 @@ TEST(IncludeCleaner, GetUnusedHeaders) {
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 @@ TEST(IncludeCleaner, VirtualBuffers) {
// 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 @@ TEST(IncludeCleaner, VirtualBuffers) {
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
More information about the cfe-commits
mailing list