ioeric updated this revision to Diff 134947.
ioeric added a comment.
- assert in the very beginning!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43462
Files:
clangd/ClangdServer.cpp
clangd/Headers.cpp
clangd/Headers.h
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/FileIndex.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===================================================================
--- unittests/clangd/HeadersTests.cpp
+++ 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<bool>(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,25 +48,25 @@
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) {
FS.Files[MainFile] = R"cpp(
#include "bar.h"
)cpp";
std::string BarHeader = testPath("sub/bar.h");
FS.Files[BarHeader] = "";
- EXPECT_EQ(shorten(BarHeader), "");
+ EXPECT_EQ(calculate(BarHeader), "");
}
TEST_F(HeadersTest, DontInsertDuplicateDifferentName) {
std::string BarHeader = testPath("sub/bar.h");
FS.Files[BarHeader] = "";
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
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ 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<ParsedAST> build(std::string Path, llvm::StringRef Code) {
+/// \p Code is put into <Path>.h which is included by \p <BasePath>.cpp.
+llvm::Optional<ParsedAST> 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::InMemoryFileSystem> 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<PCHContainerOperations>(),
- vfs::getRealFileSystem());
+ std::make_shared<PCHContainerOperations>(), 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, "<string>");
+ });
+}
+#endif
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ 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: clangd/index/FileIndex.cpp
===================================================================
--- clangd/index/FileIndex.cpp
+++ 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<SymbolSlab> indexAST(ASTContext &Ctx,
std::shared_ptr<Preprocessor> PP,
llvm::ArrayRef<const Decl *> 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<SymbolCollector>(std::move(CollectorOpts));
Collector->setPreprocessor(std::move(PP));
index::IndexingOptions IndexOpts;
Index: clangd/index/CanonicalIncludes.h
===================================================================
--- clangd/index/CanonicalIncludes.h
+++ clangd/index/CanonicalIncludes.h
@@ -23,14 +23,16 @@
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Regex.h"
+#include <mutex>
#include <string>
#include <vector>
namespace clang {
namespace clangd {
/// 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<std::pair<llvm::Regex, std::string>>
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: clangd/index/CanonicalIncludes.cpp
===================================================================
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -28,6 +28,7 @@
}
llvm::StringRef CanonicalIncludes::mapHeader(llvm::StringRef Header) const {
+ std::lock_guard<std::mutex> Lock(RegexMutex);
for (auto &Entry : RegexHeaderMappingTable) {
#ifndef NDEBUG
std::string Dummy;
Index: clangd/Headers.h
===================================================================
--- clangd/Headers.h
+++ 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 <path>. 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 <path>. 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<std::string>
-shortenIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header,
- const tooling::CompileCommand &CompileCommand,
- IntrusiveRefCntPtr<vfs::FileSystem> FS);
+calculateIncludePath(PathRef File, llvm::StringRef Code, llvm::StringRef Header,
+ const tooling::CompileCommand &CompileCommand,
+ IntrusiveRefCntPtr<vfs::FileSystem> FS);
} // namespace clangd
} // namespace clang
Index: clangd/Headers.cpp
===================================================================
--- clangd/Headers.cpp
+++ 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<std::string>
-shortenIncludePath(llvm::StringRef File, llvm::StringRef Code,
- llvm::StringRef Header,
- const tooling::CompileCommand &CompileCommand,
- IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+calculateIncludePath(llvm::StringRef File, llvm::StringRef Code,
+ llvm::StringRef Header,
+ const tooling::CompileCommand &CompileCommand,
+ IntrusiveRefCntPtr<vfs::FileSystem> 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: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ 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())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
