diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -356,7 +356,7 @@ [&SM, &CanonIncludes](FileID ID) -> Optional { const FileEntry *Entry = SM.getFileEntryForID(ID); if (Entry) { - auto PublicHeader = CanonIncludes.mapHeader(Entry->getName()); + auto PublicHeader = CanonIncludes.mapHeader(Entry); if (!PublicHeader.empty()) { return PublicHeader; } diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.h b/clang-tools-extra/clangd/index/CanonicalIncludes.h --- a/clang-tools-extra/clangd/index/CanonicalIncludes.h +++ b/clang-tools-extra/clangd/index/CanonicalIncludes.h @@ -22,6 +22,7 @@ #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/FileSystem/UniqueID.h" #include #include #include @@ -34,14 +35,14 @@ /// 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 string-to-string mapping from \p ID to \p CanonicalPath. + void addMapping(const FileEntry *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(const FileEntry *Header) const; /// Adds mapping for system headers and some special symbols (e.g. STL symbols /// in need to be mapped individually). Approximately, the following @@ -54,8 +55,8 @@ void addSystemHeadersMapping(const LangOptions &Language); private: - /// A map from full include path to a canonical path. - llvm::StringMap FullPathMapping; + /// A map from the private header to a canonical include path. + llvm::DenseMap 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 *StdSuffixHeaderMapping = nullptr; diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp --- a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp +++ b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp @@ -9,6 +9,7 @@ #include "CanonicalIncludes.h" #include "Headers.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/FileSystem/UniqueID.h" #include "llvm/Support/Path.h" #include @@ -18,18 +19,17 @@ const char IWYUPragma[] = "// IWYU pragma: private, include "; } // namespace -void CanonicalIncludes::addMapping(llvm::StringRef Path, +void CanonicalIncludes::addMapping(const FileEntry *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(const FileEntry *Header) const { + auto MapIt = FullPathMapping.find(Header->getUniqueID()); if (MapIt != FullPathMapping.end()) return MapIt->second; @@ -39,10 +39,11 @@ 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 +67,12 @@ PP.getSourceManager(), PP.getLangOpts()); if (!Text.consume_front(IWYUPragma)) return false; - // 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()); + // FIXME(ioeric): resolve the header and store actual file path. For now, + // we simply assume the written header is suitable to be #included. + auto &SM = PP.getSourceManager(); + Includes->addMapping(SM.getFileEntryForID(SM.getFileID(Range.getBegin())), + isLiteralInclude(Text) ? Text.str() + : ("\"" + Text + "\"").str()); return false; } diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -381,7 +381,7 @@ // 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(FE); 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 --- a/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp +++ b/clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp @@ -6,14 +6,27 @@ // //===----------------------------------------------------------------------===// +#include "TestFS.h" #include "index/CanonicalIncludes.h" +#include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/VirtualFileSystem.h" #include "gtest/gtest.h" namespace clang { namespace clangd { namespace { +const FileEntry * +addFile(llvm::IntrusiveRefCntPtr &FS, + llvm::IntrusiveRefCntPtr &FM, llvm::StringRef Filename) { + FS->addFile(Filename, 0, llvm::MemoryBuffer::getMemBuffer("")); + auto File = FM->getFile(Filename); + return *File; +} + TEST(CanonicalIncludesTest, CStandardLibrary) { CanonicalIncludes CI; auto Language = LangOptions(); @@ -40,29 +53,47 @@ // iosfwd declares some symbols it doesn't own. EXPECT_EQ("", CI.mapSymbol("std::ostream")); // And (for now) we assume it owns the others. - EXPECT_EQ("", CI.mapHeader("iosfwd")); + auto InMemFS = llvm::makeIntrusiveRefCnt(); + llvm::IntrusiveRefCntPtr Files( + new FileManager(FileSystemOptions(), InMemFS)); + const auto *File = addFile(InMemFS, Files, testPath("iosfwd")); + EXPECT_EQ("", CI.mapHeader(File)); } TEST(CanonicalIncludesTest, PathMapping) { + auto InMemFS = llvm::makeIntrusiveRefCnt(); + llvm::IntrusiveRefCntPtr Files( + new FileManager(FileSystemOptions(), InMemFS)); + const auto *Bar = addFile(InMemFS, Files, testPath("foo/bar")); + const auto *Other = addFile(InMemFS, Files, testPath("foo/baz")); // As used for IWYU pragmas. CanonicalIncludes CI; - CI.addMapping("foo/bar", ""); + CI.addMapping(Bar, ""); - EXPECT_EQ("", CI.mapHeader("foo/bar")); - EXPECT_EQ("", CI.mapHeader("bar/bar")); + // We added a mapping for baz. + EXPECT_EQ("", CI.mapHeader(Bar)); + // Other file doesn't have a mapping. + EXPECT_EQ("", CI.mapHeader(Other)); } TEST(CanonicalIncludesTest, Precedence) { + auto InMemFS = llvm::makeIntrusiveRefCnt(); + llvm::IntrusiveRefCntPtr Files( + new FileManager(FileSystemOptions(), InMemFS)); + const auto *File = addFile(InMemFS, Files, testPath("some/path")); + CanonicalIncludes CI; - CI.addMapping("some/path", ""); + CI.addMapping(File, ""); LangOptions Language; Language.CPlusPlus = true; CI.addSystemHeadersMapping(Language); // We added a mapping from some/path to . - ASSERT_EQ("", CI.mapHeader("some/path")); + ASSERT_EQ("", CI.mapHeader(File)); // We should have a path from 'bits/stl_vector.h' to ''. - ASSERT_EQ("", CI.mapHeader("bits/stl_vector.h")); + const auto *STLVectorFile = + addFile(InMemFS, Files, testPath("bits/stl_vector.h")); + ASSERT_EQ("", CI.mapHeader(STLVectorFile)); } } // namespace diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -1578,15 +1578,22 @@ } TEST_F(SymbolCollectorTest, SkipIncFileWhenCanonicalizeHeaders) { - CollectorOpts.CollectIncludePath = true; - CanonicalIncludes Includes; - Includes.addMapping(TestHeaderName, ""); - 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 Files( + new FileManager(FileSystemOptions(), InMemoryFileSystem)); + std::string HeaderCode = "#include \"test.inc\"\nclass Y {};"; + InMemoryFileSystem->addFile(TestHeaderName, 0, + llvm::MemoryBuffer::getMemBuffer(HeaderCode)); + auto File = Files->getFile(TestHeaderName); + ASSERT_TRUE(File); + CanonicalIncludes Includes; + Includes.addMapping(*File, ""); + CollectorOpts.CollectIncludePath = true; + CollectorOpts.Includes = &Includes; + runSymbolCollector(HeaderCode, /*Main=*/"", /*ExtraArgs=*/{"-I", testRoot()}); EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(qName("X"), declURI(IncURI),