[clang-tools-extra] 3de4d5e - [clangd] Use stable keys for CanonicalIncludes mappings
Kirill Bobyrev via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 5 07:28:08 PDT 2022
Author: Kirill Bobyrev
Date: 2022-04-05T16:27:54+02:00
New Revision: 3de4d5e6dd66165057439c69b6a03e7001ec03e0
URL: https://github.com/llvm/llvm-project/commit/3de4d5e6dd66165057439c69b6a03e7001ec03e0
DIFF: https://github.com/llvm/llvm-project/commit/3de4d5e6dd66165057439c69b6a03e7001ec03e0.diff
LOG: [clangd] Use stable keys for CanonicalIncludes mappings
This patch switches CanonicalInclude mappings to use `llvm::sys::fs::UniqueID` for a stable file representation because the `FileEntry::getName()` results turn out to be changing throughout the lifetime of a program (exposed in D120306). This patch makes it possible for D120306 to be re-landed and increases overall stability.
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D123031
Added:
Modified:
clang-tools-extra/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/clangd/index/CanonicalIncludes.h
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
index ce02f97bb7d8c..3d40219616eca 100644
--- a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
+++ b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
@@ -8,7 +8,9 @@
#include "CanonicalIncludes.h"
#include "Headers.h"
+#include "clang/Basic/FileEntry.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem/UniqueID.h"
#include "llvm/Support/Path.h"
#include <algorithm>
@@ -18,18 +20,17 @@ namespace {
const char IWYUPragma[] = "// IWYU pragma: private, include ";
} // namespace
-void CanonicalIncludes::addMapping(llvm::StringRef Path,
+void CanonicalIncludes::addMapping(FileEntryRef Header,
llvm::StringRef CanonicalPath) {
- FullPathMapping[Path] = std::string(CanonicalPath);
+ FullPathMapping[Header.getUniqueID()] = std::string(CanonicalPath);
}
/// The maximum number of path components in a key from StdSuffixHeaderMapping.
/// Used to minimize the number of lookups in suffix path mappings.
constexpr int MaxSuffixComponents = 3;
-llvm::StringRef CanonicalIncludes::mapHeader(llvm::StringRef Header) const {
- assert(!Header.empty());
- auto MapIt = FullPathMapping.find(Header);
+llvm::StringRef CanonicalIncludes::mapHeader(FileEntryRef Header) const {
+ auto MapIt = FullPathMapping.find(Header.getUniqueID());
if (MapIt != FullPathMapping.end())
return MapIt->second;
@@ -39,10 +40,11 @@ llvm::StringRef CanonicalIncludes::mapHeader(llvm::StringRef Header) const {
int Components = 1;
// FIXME: check that this works on Windows and add tests.
- for (auto It = llvm::sys::path::rbegin(Header),
- End = llvm::sys::path::rend(Header);
+ auto Filename = Header.getName();
+ for (auto It = llvm::sys::path::rbegin(Filename),
+ End = llvm::sys::path::rend(Filename);
It != End && Components <= MaxSuffixComponents; ++It, ++Components) {
- auto SubPath = Header.substr(It->data() - Header.begin());
+ auto SubPath = Filename.substr(It->data() - Filename.begin());
auto MappingIt = StdSuffixHeaderMapping->find(SubPath);
if (MappingIt != StdSuffixHeaderMapping->end())
return MappingIt->second;
@@ -66,12 +68,12 @@ collectIWYUHeaderMaps(CanonicalIncludes *Includes) {
PP.getSourceManager(), PP.getLangOpts());
if (!Text.consume_front(IWYUPragma))
return false;
+ auto &SM = PP.getSourceManager();
// We always insert using the spelling from the pragma.
- if (auto *FE = PP.getSourceManager().getFileEntryForID(
- PP.getSourceManager().getFileID(Range.getBegin())))
- Includes->addMapping(FE->getName(), isLiteralInclude(Text)
- ? Text.str()
- : ("\"" + Text + "\"").str());
+ if (auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin())))
+ Includes->addMapping(
+ FE->getLastRef(),
+ isLiteralInclude(Text) ? Text.str() : ("\"" + Text + "\"").str());
return false;
}
diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.h b/clang-tools-extra/clangd/index/CanonicalIncludes.h
index 747c3f1309573..c3ac9c3b80623 100644
--- a/clang-tools-extra/clangd/index/CanonicalIncludes.h
+++ b/clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -19,9 +19,11 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_CANONICALINCLUDES_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_CANONICALINCLUDES_H
+#include "clang/Basic/FileEntry.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem/UniqueID.h"
#include <mutex>
#include <string>
#include <vector>
@@ -34,14 +36,14 @@ namespace clangd {
/// Only const methods (i.e. mapHeader) in this class are thread safe.
class CanonicalIncludes {
public:
- /// Adds a string-to-string mapping from \p Path to \p CanonicalPath.
- void addMapping(llvm::StringRef Path, llvm::StringRef CanonicalPath);
+ /// Adds a file-to-string mapping from \p ID to \p CanonicalPath.
+ void addMapping(FileEntryRef Header, llvm::StringRef CanonicalPath);
/// Returns the overridden include for symbol with \p QualifiedName, or "".
llvm::StringRef mapSymbol(llvm::StringRef QualifiedName) const;
/// Returns the overridden include for for files in \p Header, or "".
- llvm::StringRef mapHeader(llvm::StringRef Header) const;
+ llvm::StringRef mapHeader(FileEntryRef Header) const;
/// Adds mapping for system headers and some special symbols (e.g. STL symbols
/// in <iosfwd> need to be mapped individually). Approximately, the following
@@ -54,8 +56,8 @@ class CanonicalIncludes {
void addSystemHeadersMapping(const LangOptions &Language);
private:
- /// A map from full include path to a canonical path.
- llvm::StringMap<std::string> FullPathMapping;
+ /// A map from the private header to a canonical include path.
+ llvm::DenseMap<llvm::sys::fs::UniqueID, std::string> FullPathMapping;
/// A map from a suffix (one or components of a path) to a canonical path.
/// Used only for mapping standard headers.
const llvm::StringMap<llvm::StringRef> *StdSuffixHeaderMapping = nullptr;
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index e7923bae0c43c..770477220e9be 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -381,7 +381,8 @@ class SymbolCollector::HeaderFileURICache {
// If a file is mapped by canonical headers, use that mapping, regardless
// of whether it's an otherwise-good header (header guards etc).
if (Includes) {
- llvm::StringRef Canonical = Includes->mapHeader(Filename);
+ llvm::StringRef Canonical =
+ Includes->mapHeader(*SM.getFileEntryRefForID(FID));
if (!Canonical.empty()) {
// If we had a mapping, always use it.
if (Canonical.startswith("<") || Canonical.startswith("\""))
diff --git a/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp b/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
index 31f99b7615d32..20e01d31b29a7 100644
--- a/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -6,14 +6,30 @@
//
//===----------------------------------------------------------------------===//
+#include "TestFS.h"
#include "index/CanonicalIncludes.h"
+#include "clang/Basic/FileEntry.h"
+#include "clang/Basic/FileManager.h"
#include "clang/Basic/LangOptions.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
namespace clang {
namespace clangd {
namespace {
+FileEntryRef addFile(llvm::vfs::InMemoryFileSystem &FS, FileManager &FM,
+ llvm::StringRef Filename) {
+ FS.addFile(Filename, 0, llvm::MemoryBuffer::getMemBuffer(""));
+ auto File = FM.getFileRef(Filename);
+ EXPECT_THAT_EXPECTED(File, llvm::Succeeded());
+ return *File;
+}
+
TEST(CanonicalIncludesTest, CStandardLibrary) {
CanonicalIncludes CI;
auto Language = LangOptions();
@@ -40,29 +56,52 @@ TEST(CanonicalIncludesTest, CXXStandardLibrary) {
// iosfwd declares some symbols it doesn't own.
EXPECT_EQ("<ostream>", CI.mapSymbol("std::ostream"));
// And (for now) we assume it owns the others.
- EXPECT_EQ("<iosfwd>", CI.mapHeader("iosfwd"));
+ auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ FileManager Files(FileSystemOptions(), InMemFS);
+ auto File = addFile(*InMemFS, Files, testPath("iosfwd"));
+ EXPECT_EQ("<iosfwd>", CI.mapHeader(File));
}
TEST(CanonicalIncludesTest, PathMapping) {
+ auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ FileManager Files(FileSystemOptions(), InMemFS);
+ std::string BarPath = testPath("foo/bar");
+ auto Bar = addFile(*InMemFS, Files, BarPath);
+ auto Other = addFile(*InMemFS, Files, testPath("foo/baz"));
// As used for IWYU pragmas.
CanonicalIncludes CI;
- CI.addMapping("foo/bar", "<baz>");
+ CI.addMapping(Bar, "<baz>");
- EXPECT_EQ("<baz>", CI.mapHeader("foo/bar"));
- EXPECT_EQ("", CI.mapHeader("bar/bar"));
+ // We added a mapping for baz.
+ EXPECT_EQ("<baz>", CI.mapHeader(Bar));
+ // Other file doesn't have a mapping.
+ EXPECT_EQ("", CI.mapHeader(Other));
+
+ // Add hard link to "foo/bar" and check that it is also mapped to <baz>, hence
+ // does not depend on the header name.
+ std::string HardLinkPath = testPath("hard/link");
+ InMemFS->addHardLink(HardLinkPath, BarPath);
+ auto HardLinkFile = Files.getFileRef(HardLinkPath);
+ ASSERT_THAT_EXPECTED(HardLinkFile, llvm::Succeeded());
+ EXPECT_EQ("<baz>", CI.mapHeader(*HardLinkFile));
}
TEST(CanonicalIncludesTest, Precedence) {
+ auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ FileManager Files(FileSystemOptions(), InMemFS);
+ auto File = addFile(*InMemFS, Files, testPath("some/path"));
+
CanonicalIncludes CI;
- CI.addMapping("some/path", "<path>");
+ CI.addMapping(File, "<path>");
LangOptions Language;
Language.CPlusPlus = true;
CI.addSystemHeadersMapping(Language);
// We added a mapping from some/path to <path>.
- ASSERT_EQ("<path>", CI.mapHeader("some/path"));
+ ASSERT_EQ("<path>", CI.mapHeader(File));
// We should have a path from 'bits/stl_vector.h' to '<vector>'.
- ASSERT_EQ("<vector>", CI.mapHeader("bits/stl_vector.h"));
+ auto STLVectorFile = addFile(*InMemFS, Files, testPath("bits/stl_vector.h"));
+ ASSERT_EQ("<vector>", CI.mapHeader(STLVectorFile));
}
} // namespace
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 7c1e1a96db003..93d324d1de446 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock-matchers.h"
#include "gmock/gmock-more-matchers.h"
#include "gmock/gmock.h"
@@ -1578,15 +1579,22 @@ TEST_F(SymbolCollectorTest, IWYUPragmaWithDoubleQuotes) {
}
TEST_F(SymbolCollectorTest, SkipIncFileWhenCanonicalizeHeaders) {
- CollectorOpts.CollectIncludePath = true;
- CanonicalIncludes Includes;
- Includes.addMapping(TestHeaderName, "<canonical>");
- CollectorOpts.Includes = &Includes;
auto IncFile = testPath("test.inc");
auto IncURI = URI::create(IncFile).toString();
InMemoryFileSystem->addFile(IncFile, 0,
llvm::MemoryBuffer::getMemBuffer("class X {};"));
- runSymbolCollector("#include \"test.inc\"\nclass Y {};", /*Main=*/"",
+ llvm::IntrusiveRefCntPtr<FileManager> Files(
+ new FileManager(FileSystemOptions(), InMemoryFileSystem));
+ std::string HeaderCode = "#include \"test.inc\"\nclass Y {};";
+ InMemoryFileSystem->addFile(TestHeaderName, 0,
+ llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+ auto File = Files->getFileRef(TestHeaderName);
+ ASSERT_THAT_EXPECTED(File, llvm::Succeeded());
+ CanonicalIncludes Includes;
+ Includes.addMapping(*File, "<canonical>");
+ CollectorOpts.CollectIncludePath = true;
+ CollectorOpts.Includes = &Includes;
+ runSymbolCollector(HeaderCode, /*Main=*/"",
/*ExtraArgs=*/{"-I", testRoot()});
EXPECT_THAT(Symbols,
UnorderedElementsAre(AllOf(qName("X"), declURI(IncURI),
More information about the cfe-commits
mailing list