[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE325523: [clangd] Fixes for #include insertion. (authored 
by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43462?vs=134947=134952#toc

Repository:
  rL LLVM

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: 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())
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
-shortenIncludePath(llvm::StringRef File, llvm::StringRef Code,
-   llvm::StringRef Header,
-   const tooling::CompileCommand ,
-   IntrusiveRefCntPtr FS) {
+calculateIncludePath(llvm::StringRef File, llvm::StringRef Code,
+ llvm::StringRef Header,
+ const tooling::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: 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 indexAST(ASTContext ,
  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: 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 
 #include 
 #include 
 
 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;
@@ 

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325523: [clangd] Fixes for #include insertion. (authored by 
ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43462

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/Headers.cpp
  clang-tools-extra/trunk/clangd/Headers.h
  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
  clang-tools-extra/trunk/clangd/index/FileIndex.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

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 ) {
+EXPECT_EQ(Sym.Detail->IncludeHeader, "");
+  });
+}
+#endif
+
 } // namespace
 } // namespace clangd
 } // namespace clang
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/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));
 

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
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(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 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");
+  

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric updated this revision to Diff 134944.
ioeric marked 2 inline comments as done.
ioeric added a comment.

- added a comment about thread safety


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(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 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;
+  

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric marked 5 inline comments as done.
ioeric added inline comments.



Comment at: clangd/Headers.cpp:60
 Argv.push_back(S.c_str());
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = clang::createInvocationFromCommandLine(

ilya-biryukov wrote:
> Not related to this patch, but just noticed that we don't call 
> `FS->setCurrentWorkingDirectory(CompileCommand.Directory)` here.
> This might break if `CompileCommand` has non-absolute paths.
This was done in ClangdServer, but we should probably do it here as you 
suggested.



Comment at: clangd/index/FileIndex.cpp:36
+  CollectorOpts.IndexMainFiles = false;
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();

sammccall wrote:
> if this is always true now, we could remove the option?
Will do this in a followup patch. Added a FIXME



Comment at: clangd/index/FileIndex.cpp:37
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;

sammccall wrote:
> This is not going to handle IWYU right?
> It seems like at this point the right thing to do is totally hide the 
> CanonicalIncludes inside SymbolCollector, which would init it (to system 
> headers), write IWYU mappings using the PP, and consume the mappings (when 
> emitting symbols).
> Churn :(
You are right :(

I'll prepare a followup patch to handle this. Added a FIXME.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric updated this revision to Diff 134943.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- addressed comments.


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(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 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 = 

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/index/CanonicalIncludes.cpp:21
llvm::StringRef CanonicalPath) {
   addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
   CanonicalPath);

ilya-biryukov wrote:
> Technically, if one thread initializes `CanonicalIncludes` and the other 
> thread reads from it, this is a data race.
> Maybe we're ok with our current usage, but I don't have enough confidence in 
> that (i.e. don't know C++'s memory model good enough) and would advice to 
> take the lock in other functions of `CanonicalIncludes` too.
> 
As Sam pointed out, it's fine the way it is right now. We don't want any 
thread-safety guarantees from the non-const methods of the class.
But could you please add a comment to the class that non-const methods of it 
are not thread-safe even though it has a `mutex` inside?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/Headers.cpp:60
 Argv.push_back(S.c_str());
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = clang::createInvocationFromCommandLine(

Not related to this patch, but just noticed that we don't call 
`FS->setCurrentWorkingDirectory(CompileCommand.Directory)` here.
This might break if `CompileCommand` has non-absolute paths.



Comment at: clangd/index/CanonicalIncludes.cpp:21
llvm::StringRef CanonicalPath) {
   addRegexMapping((llvm::Twine("^") + llvm::Regex::escape(Path) + "$").str(),
   CanonicalPath);

Technically, if one thread initializes `CanonicalIncludes` and the other thread 
reads from it, this is a data race.
Maybe we're ok with our current usage, but I don't have enough confidence in 
that (i.e. don't know C++'s memory model good enough) and would advice to take 
the lock in other functions of `CanonicalIncludes` too.




Comment at: clangd/index/FileIndex.cpp:18
 
+const CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  static const auto *Includes = [] {

NIT: `lowerCase` the function name.



Comment at: clangd/index/FileIndex.cpp:38
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;
+

NIT: remove local var, replace with `CollectorOpts.Includes = 
CanonicalIncludesForSystemHeaders()`?



Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {

ioeric wrote:
> ilya-biryukov wrote:
> > Why not on Windows?
> because our system header map doesn't really support windows...
Thanks for clarification.
I thought it's standard-library specific, not OS-specific. I would have 
expected it to work on Windows with libstdc++. (which we're "mocking" here).
Anyway, this not related to this patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clangd/Headers.h:26
 ///
 /// \param Header is an absolute file path.
+/// \return A quoted "path" or . This returns an empty string if:

File also needs to be absolute.
(May want to add asserts for this at the start of the impl - up to you)



Comment at: clangd/index/FileIndex.cpp:36
+  CollectorOpts.IndexMainFiles = false;
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();

if this is always true now, we could remove the option?



Comment at: clangd/index/FileIndex.cpp:37
+  CollectorOpts.CollectIncludePath = true;
+  const auto *Includes = CanonicalIncludesForSystemHeaders();
+  CollectorOpts.Includes = Includes;

This is not going to handle IWYU right?
It seems like at this point the right thing to do is totally hide the 
CanonicalIncludes inside SymbolCollector, which would init it (to system 
headers), write IWYU mappings using the PP, and consume the mappings (when 
emitting symbols).
Churn :(


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric updated this revision to Diff 134932.
ioeric marked 6 inline comments as done.
ioeric added a comment.

- Stop indexing main files in dynamic index; addressed review comments.


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(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 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, 

[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric added inline comments.



Comment at: clangd/Headers.cpp:45
+bool hasHeaderExtension(PathRef Path) {
+  constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh",
+ ".hxx"};

ilya-biryukov wrote:
> There is another list of header extensions in 
> `ClangdServer::switchSourceHeader`, let's reuse the list of extensions used 
> here and there.
Acked. (The change is not relevant anymore.)



Comment at: clangd/index/FileIndex.cpp:18
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();

ilya-biryukov wrote:
> Maybe store a static inside the function body (to properly free memory) and 
> return a const reference (to avoid mutating shared data)?
> 
> ```
> const CanonicalIncludes () {
>   static CanonicalInclude Includes = []() -> CanonicalHeader {
> CanonicalInclude Includes;
> addSystemHeadersMapping(Includes);
> return Includes;
>   };
>   return Includes;
> }
> ```
> 
> Also Sam mentioned that `CanonicalIncludes` is not thread-safe, as the 
> `Regex`'s `match()` mutates.
As discussed offline, we want to avoid destructing static objects because ... 

Made `CanonicalIncludes` thread-safe by adding a mutex around matches.



Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {

ilya-biryukov wrote:
> Why not on Windows?
because our system header map doesn't really support windows...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Sam McCall via Phabricator via cfe-commits
sammccall added inline comments.



Comment at: clangd/Headers.cpp:42-43
 };
 
+/// Returns true if \p Path has header extensions like .h and .hpp etc.
+bool hasHeaderExtension(PathRef Path) {

As discussed offline, this seems dubious.
- this is probably the generalization of '.inc' handling, and so probably wants 
the include stack and belongs at indexing time (we're determining "what's the 
public header for this symbol, or is the symbol private")?
 - as discussed offline, it seems like we can sidestep this whole problem by 
not indexing symbols only visible in main files, for now
(later we'll want a flag on the symbol for whether it's public, so we can 
support find-symbol-by-name for mainfile-declared symbols)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments.



Comment at: clangd/Headers.cpp:45
+bool hasHeaderExtension(PathRef Path) {
+  constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh",
+ ".hxx"};

There is another list of header extensions in 
`ClangdServer::switchSourceHeader`, let's reuse the list of extensions used 
here and there.



Comment at: clangd/Headers.cpp:48
+  return std::any_of(std::begin(HeaderExtensions), std::end(HeaderExtensions),
+ [](const char *Ext) { return Path.endswith(Ext); });
+}

Maybe use `llvm::sys::path::extension` and search it in `HeaderExtensions` 
using `StringRef::equals_lower`?
Will also handle upper- and mixed-case.



Comment at: clangd/Headers.cpp:60
+ IntrusiveRefCntPtr FS) {
+  if (File == Header || !hasHeaderExtension(Header))
+return "";

Maybe replace `!hasHeaderExtension(Header)` to `hasSourceExtension(Header)`?
Knowing about all header extensions in advance is hard, knowing a subset of 
source extensions seems totally true.



Comment at: clangd/index/FileIndex.cpp:18
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();

Maybe store a static inside the function body (to properly free memory) and 
return a const reference (to avoid mutating shared data)?

```
const CanonicalIncludes () {
  static CanonicalInclude Includes = []() -> CanonicalHeader {
CanonicalInclude Includes;
addSystemHeadersMapping(Includes);
return Includes;
  };
  return Includes;
}
```

Also Sam mentioned that `CanonicalIncludes` is not thread-safe, as the 
`Regex`'s `match()` mutates.



Comment at: unittests/clangd/FileIndexTests.cpp:173
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {

Why not on Windows?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D43462: [clangd] Fixes for #include insertion.

2018-02-19 Thread Eric Liu via Phabricator via cfe-commits
ioeric created this revision.
ioeric added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, jkorous-apple, klimek.

o Avoid inserting a header include into the header itself.
o Avoid inserting non-header files.
o Canonicalize include paths for symbols in dynamic index.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43462

Files:
  clangd/ClangdServer.cpp
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/index/FileIndex.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(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,19 @@
   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), "");
+}
+
+TEST_F(HeadersTest, DoNotInsertCppFile) {
+  std::string CppFile = testPath("sub/x.cpp");
+  FS.Files[CppFile] = "";
+  EXPECT_EQ(calculate(CppFile), "");
 }
 
 } // 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"
@@ -169,6 +170,20 @@
   EXPECT_THAT(match(M, Req), UnorderedElementsAre("X"));
 }
 
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
+  FileIndex M;
+  std::string File = testPath("bits/basic_string.h");
+  M.update(File, build(File, "class string {};").getPointer());
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  M.fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.Detail->IncludeHeader, "");
+  });
+}
+#endif
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -15,14 +15,24 @@
 namespace clangd {
 namespace {
 
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();
+  addSystemHeadersMapping(Includes);
+  return Includes;
+}
+
 /// Retrieves namespace and class level symbols in \p Decls.
 std::unique_ptr indexAST(ASTContext ,
  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;
+