Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp @@ -327,13 +327,11 @@ if (!Resolved) return Resolved.takeError(); - auto FS = FSProvider.getTaggedFileSystem(File).Value; tooling::CompileCommand CompileCommand = CompileArgs.getCompileCommand(File); - FS->setCurrentWorkingDirectory(CompileCommand.Directory); - auto Include = - shortenIncludePath(File, Code, *Resolved, CompileCommand, FS); + calculateIncludePath(File, Code, *Resolved, CompileCommand, + FSProvider.getTaggedFileSystem(File).Value); if (!Include) return Include.takeError(); if (Include->empty()) Index: clang-tools-extra/trunk/clangd/Headers.h =================================================================== --- clang-tools-extra/trunk/clangd/Headers.h +++ clang-tools-extra/trunk/clangd/Headers.h @@ -18,18 +18,21 @@ namespace clang { namespace clangd { + /// Determines the preferred way to #include a file, taking into account the /// search path. Usually this will prefer a shorter representation like /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'. /// +/// \param File is an absolute file path. /// \param Header is an absolute file path. -/// \return A quoted "path" or . If \p Header is already (directly) -/// included in the file (including those included via different paths), this -/// returns an empty string. +/// \return A quoted "path" or . This returns an empty string if: +/// - \p Header is already (directly) included in the file (including those +/// included via different paths). +/// - \p Header is the same as \p File. llvm::Expected -shortenIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header, - const tooling::CompileCommand &CompileCommand, - IntrusiveRefCntPtr FS); +calculateIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header, + const tooling::CompileCommand &CompileCommand, + IntrusiveRefCntPtr FS); } // namespace clangd } // namespace clang Index: clang-tools-extra/trunk/clangd/Headers.cpp =================================================================== --- clang-tools-extra/trunk/clangd/Headers.cpp +++ clang-tools-extra/trunk/clangd/Headers.cpp @@ -16,6 +16,7 @@ #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/PreprocessorOptions.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/Support/Path.h" namespace clang { namespace clangd { @@ -45,10 +46,17 @@ /// FIXME(ioeric): we might not want to insert an absolute include path if the /// path is not shortened. llvm::Expected -shortenIncludePath(llvm::StringRef File, llvm::StringRef Code, - llvm::StringRef Header, - const tooling::CompileCommand &CompileCommand, - IntrusiveRefCntPtr FS) { +calculateIncludePath(llvm::StringRef File, llvm::StringRef Code, + llvm::StringRef Header, + const tooling::CompileCommand &CompileCommand, + IntrusiveRefCntPtr FS) { + assert(llvm::sys::path::is_absolute(File) && + llvm::sys::path::is_absolute(Header)); + + if (File == Header) + return ""; + FS->setCurrentWorkingDirectory(CompileCommand.Directory); + // Set up a CompilerInstance and create a preprocessor to collect existing // #include headers in \p Code. Preprocesor also provides HeaderSearch with // which we can calculate the shortest include path for \p Header. Index: clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h =================================================================== --- clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h +++ clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h @@ -23,6 +23,7 @@ #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/StringMap.h" #include "llvm/Support/Regex.h" +#include #include #include @@ -31,6 +32,7 @@ /// Maps a definition location onto an #include file, based on a set of filename /// rules. +/// Only const methods (i.e. mapHeader) in this class are thread safe. class CanonicalIncludes { public: CanonicalIncludes() = default; @@ -53,6 +55,8 @@ // arbitrary regexes. mutable std::vector> RegexHeaderMappingTable; + // Guards Regex matching as it's not thread-safe. + mutable std::mutex RegexMutex; }; /// Returns a CommentHandler that parses pragma comment on include files to Index: clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp =================================================================== --- clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp +++ clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp @@ -28,6 +28,7 @@ } llvm::StringRef CanonicalIncludes::mapHeader(llvm::StringRef Header) const { + std::lock_guard Lock(RegexMutex); for (auto &Entry : RegexHeaderMappingTable) { #ifndef NDEBUG std::string Dummy; Index: clang-tools-extra/trunk/clangd/index/FileIndex.cpp =================================================================== --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp @@ -15,14 +15,30 @@ namespace clangd { namespace { +const CanonicalIncludes *canonicalIncludesForSystemHeaders() { + static const auto *Includes = [] { + auto *I = new CanonicalIncludes(); + addSystemHeadersMapping(I); + return I; + }(); + return Includes; +} + /// Retrieves namespace and class level symbols in \p Decls. std::unique_ptr indexAST(ASTContext &Ctx, std::shared_ptr PP, llvm::ArrayRef Decls) { SymbolCollector::Options CollectorOpts; - // Code completion gets main-file results from Sema. - // But we leave this option on because features like go-to-definition want it. - CollectorOpts.IndexMainFiles = true; + // Although we do not index symbols in main files (e.g. cpp file), information + // in main files like definition locations of class declarations will still be + // collected; thus, the index works for go-to-definition. + // FIXME(ioeric): handle IWYU pragma for dynamic index. We might want to make + // SymbolCollector always provides include canonicalization (e.g. IWYU, STL). + // FIXME(ioeric): get rid of `IndexMainFiles` as this is always set to false. + CollectorOpts.IndexMainFiles = false; + CollectorOpts.CollectIncludePath = true; + CollectorOpts.Includes = canonicalIncludesForSystemHeaders(); + auto Collector = std::make_shared(std::move(CollectorOpts)); Collector->setPreprocessor(std::move(PP)); index::IndexingOptions IndexOpts; Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp @@ -580,8 +580,11 @@ /*StorePreamblesInMemory=*/true, /*BuildDynamicSymbolIndex=*/true); - Server.addDocument(testPath("foo.cpp"), R"cpp( + FS.Files[testPath("foo.h")] = R"cpp( namespace ns { class XYZ {}; void foo(int x) {} } + )cpp"; + Server.addDocument(testPath("foo.cpp"), R"cpp( + #include "foo.h" )cpp"); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for preamble"; Index: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp @@ -7,6 +7,7 @@ // //===----------------------------------------------------------------------===// +#include "TestFS.h" #include "index/FileIndex.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PCHContainerOperations.h" @@ -83,17 +84,28 @@ } /// Create an ParsedAST for \p Code. Returns None if \p Code is empty. -llvm::Optional build(std::string Path, llvm::StringRef Code) { +/// \p Code is put into .h which is included by \p .cpp. +llvm::Optional build(llvm::StringRef BasePath, + llvm::StringRef Code) { if (Code.empty()) return llvm::None; - const char *Args[] = {"clang", "-xc++", Path.c_str()}; + + assert(llvm::sys::path::extension(BasePath).empty() && + "BasePath must be a base file path without extension."); + llvm::IntrusiveRefCntPtr VFS( + new vfs::InMemoryFileSystem); + std::string Path = (BasePath + ".cpp").str(); + std::string Header = (BasePath + ".h").str(); + VFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("")); + VFS->addFile(Header, 0, llvm::MemoryBuffer::getMemBuffer(Code)); + const char *Args[] = {"clang", "-xc++", "-include", Header.c_str(), + Path.c_str()}; auto CI = createInvocationFromCommandLine(Args); auto Buf = llvm::MemoryBuffer::getMemBuffer(Code); auto AST = ParsedAST::Build(std::move(CI), nullptr, std::move(Buf), - std::make_shared(), - vfs::getRealFileSystem()); + std::make_shared(), VFS); assert(AST.hasValue()); return std::move(*AST); } @@ -169,6 +181,20 @@ EXPECT_THAT(match(M, Req), UnorderedElementsAre("X")); } +#ifndef LLVM_ON_WIN32 +TEST(FileIndexTest, CanonicalizeSystemHeader) { + FileIndex M; + std::string File = testPath("bits/basic_string"); + M.update(File, build(File, "class string {};").getPointer()); + + FuzzyFindRequest Req; + Req.Query = ""; + M.fuzzyFind(Req, [&](const Symbol &Sym) { + EXPECT_EQ(Sym.Detail->IncludeHeader, ""); + }); +} +#endif + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp @@ -24,24 +24,14 @@ } protected: - // Adds \p File with content \p Code to the sub directory and returns the - // absolute path. -// std::string addToSubdir(PathRef File, llvm::StringRef Code = "") { -// assert(llvm::sys::path::is_relative(File) && "FileName should be relative"); -// llvm::SmallString<32> Path; -// llvm::sys::path::append(Path, Subdir, File); -// FS.Files[Path] = Code; -// return Path.str(); -// }; - - // Shortens the header and returns "" on error. - std::string shorten(PathRef Header) { + // Calculates the include path for \p Header, or returns "" on error. + std::string calculate(PathRef Header) { auto VFS = FS.getTaggedFileSystem(MainFile).Value; auto Cmd = CDB.getCompileCommand(MainFile); assert(static_cast(Cmd)); VFS->setCurrentWorkingDirectory(Cmd->Directory); auto Path = - shortenIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS); + calculateIncludePath(MainFile, FS.Files[MainFile], Header, *Cmd, VFS); if (!Path) { llvm::consumeError(Path.takeError()); return std::string(); @@ -58,7 +48,7 @@ TEST_F(HeadersTest, InsertInclude) { std::string Path = testPath("sub/bar.h"); FS.Files[Path] = ""; - EXPECT_EQ(shorten(Path), "\"bar.h\""); + EXPECT_EQ(calculate(Path), "\"bar.h\""); } TEST_F(HeadersTest, DontInsertDuplicateSameName) { @@ -67,7 +57,7 @@ )cpp"; std::string BarHeader = testPath("sub/bar.h"); FS.Files[BarHeader] = ""; - EXPECT_EQ(shorten(BarHeader), ""); + EXPECT_EQ(calculate(BarHeader), ""); } TEST_F(HeadersTest, DontInsertDuplicateDifferentName) { @@ -76,7 +66,7 @@ FS.Files[MainFile] = R"cpp( #include "sub/bar.h" // not shortest )cpp"; - EXPECT_EQ(shorten(BarHeader), ""); + EXPECT_EQ(calculate(BarHeader), ""); } TEST_F(HeadersTest, StillInsertIfTrasitivelyIncluded) { @@ -89,7 +79,13 @@ FS.Files[MainFile] = R"cpp( #include "bar.h" )cpp"; - EXPECT_EQ(shorten(BazHeader), "\"baz.h\""); + EXPECT_EQ(calculate(BazHeader), "\"baz.h\""); +} + +TEST_F(HeadersTest, DoNotInsertIfInSameFile) { + MainFile = testPath("main.h"); + FS.Files[MainFile] = ""; + EXPECT_EQ(calculate(MainFile), ""); } } // namespace