subject:"\[PATCH\] D53288\: \[clangd\] Optionally use dex for the preamble parts of the dynamic index."

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

2018-10-16 Thread Sam McCall via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rL344594: [clangd] Optionally use dex for the preamble parts 
of the dynamic index. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53288?vs=169702=169795#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53288

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/index/FileIndex.cpp
  clang-tools-extra/trunk/clangd/index/FileIndex.h
  clang-tools-extra/trunk/clangd/index/dex/Dex.h
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
  clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/TestTU.cpp

Index: clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
@@ -492,19 +492,13 @@
"other::A"));
 }
 
-// FIXME(kbobyrev): This test is different for Dex and MemIndex: while
-// MemIndex manages response deduplication, Dex simply returns all matched
-// symbols which means there might be equivalent symbols in the response.
-// Before drop-in replacement of MemIndex with Dex happens, FileIndex
-// should handle deduplication instead.
 TEST(DexTest, DexDeduplicate) {
   std::vector Symbols = {symbol("1"), symbol("2"), symbol("3"),
  symbol("2") /* duplicate */};
   FuzzyFindRequest Req;
   Req.Query = "2";
   Dex I(Symbols, RefSlab(), URISchemes);
-  EXPECT_FALSE(Req.Limit);
-  EXPECT_THAT(match(I, Req), ElementsAre("2", "2"));
+  EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(DexTest, DexLimitedNumMatches) {
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
@@ -82,37 +82,38 @@
 
 TEST(FileSymbolsTest, UpdateAndGet) {
   FileSymbols FS;
-  EXPECT_THAT(runFuzzyFind(*FS.buildMemIndex(), ""), IsEmpty());
+  EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());
 
   FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"));
-  EXPECT_THAT(runFuzzyFind(*FS.buildMemIndex(), ""),
+  EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""),
   UnorderedElementsAre(QName("1"), QName("2"), QName("3")));
-  EXPECT_THAT(getRefs(*FS.buildMemIndex(), SymbolID("1")),
+  EXPECT_THAT(getRefs(*FS.buildIndex(IndexType::Light), SymbolID("1")),
   RefsAre({FileURI("f1.cc")}));
 }
 
 TEST(FileSymbolsTest, Overlap) {
   FileSymbols FS;
   FS.update("f1", numSlab(1, 3), nullptr);
   FS.update("f2", numSlab(3, 5), nullptr);
-  EXPECT_THAT(runFuzzyFind(*FS.buildMemIndex(), ""),
-  UnorderedElementsAre(QName("1"), QName("2"), QName("3"),
-   QName("4"), QName("5")));
+  for (auto Type : {IndexType::Light, IndexType::Heavy})
+EXPECT_THAT(runFuzzyFind(*FS.buildIndex(Type), ""),
+UnorderedElementsAre(QName("1"), QName("2"), QName("3"),
+ QName("4"), QName("5")));
 }
 
 TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
   FileSymbols FS;
 
   SymbolID ID("1");
   FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"));
 
-  auto Symbols = FS.buildMemIndex();
+  auto Symbols = FS.buildIndex(IndexType::Light);
   EXPECT_THAT(runFuzzyFind(*Symbols, ""),
   UnorderedElementsAre(QName("1"), QName("2"), QName("3")));
   EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")}));
 
   FS.update("f1", nullptr, nullptr);
-  auto Empty = FS.buildMemIndex();
+  auto Empty = FS.buildIndex(IndexType::Light);
   EXPECT_THAT(runFuzzyFind(*Empty, ""), IsEmpty());
   EXPECT_THAT(getRefs(*Empty, ID), ElementsAre());
 
Index: clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
@@ -50,7 +50,8 @@
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
-  auto Idx = llvm::make_unique();
+  auto Idx = llvm::make_unique(
+  /*URISchemes=*/std::vector{}, /*UseDex=*/true);
   Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
---

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

2018-10-15 Thread Haojian Wu via Phabricator via cfe-commits

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clangd/ClangdServer.h:79
+/// Use a heavier and faster in-memory index implementation.
+/// FIXME: we should make this true if it isn't too slow!.
+bool HeavyweightDynamicSymbolIndex = false;

"too slow" seems confusing, dex is faster, I think here it means too slow to 
build?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53288



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

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

2018-10-15 Thread Sam McCall via Phabricator via cfe-commits

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, ilya-biryukov.

Reuse the old -use-dex-index experiment flag for this.

To avoid breaking the tests, make Dex deduplicate symbols, addressing an old 
FIXME.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53288

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/index/Background.cpp
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/dex/Dex.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TestTU.cpp

Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -50,7 +50,8 @@
 
 std::unique_ptr TestTU::index() const {
   auto AST = build();
-  auto Idx = llvm::make_unique();
+  auto Idx = llvm::make_unique(
+  /*URISchemes=*/std::vector{}, /*UseDex=*/true);
   Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr());
   Idx->updateMain(Filename, AST);
   return std::move(Idx);
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -86,35 +86,37 @@
 
 TEST(FileSymbolsTest, UpdateAndGet) {
   FileSymbols FS;
-  EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), UnorderedElementsAre());
+  EXPECT_THAT(getSymbolNames(*FS.buildIndex(IndexType::Light)),
+  UnorderedElementsAre());
 
   FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"));
-  EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()),
+  EXPECT_THAT(getSymbolNames(*FS.buildIndex(IndexType::Light)),
   UnorderedElementsAre("1", "2", "3"));
-  EXPECT_THAT(getRefs(*FS.buildMemIndex(), SymbolID("1")),
+  EXPECT_THAT(getRefs(*FS.buildIndex(IndexType::Light), SymbolID("1")),
   RefsAre({FileURI("f1.cc")}));
 }
 
 TEST(FileSymbolsTest, Overlap) {
   FileSymbols FS;
   FS.update("f1", numSlab(1, 3), nullptr);
   FS.update("f2", numSlab(3, 5), nullptr);
-  EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()),
-  UnorderedElementsAre("1", "2", "3", "4", "5"));
+  for (auto Type : {IndexType::Light, IndexType::Heavy})
+EXPECT_THAT(getSymbolNames(*FS.buildIndex(Type)),
+UnorderedElementsAre("1", "2", "3", "4", "5"));
 }
 
 TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
   FileSymbols FS;
 
   SymbolID ID("1");
   FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"));
 
-  auto Symbols = FS.buildMemIndex();
+  auto Symbols = FS.buildIndex(IndexType::Light);
   EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
   EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")}));
 
   FS.update("f1", nullptr, nullptr);
-  auto Empty = FS.buildMemIndex();
+  auto Empty = FS.buildIndex(IndexType::Light);
   EXPECT_THAT(getSymbolNames(*Empty), UnorderedElementsAre());
   EXPECT_THAT(getRefs(*Empty, ID), ElementsAre());
 
Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -492,19 +492,13 @@
"other::A"));
 }
 
-// FIXME(kbobyrev): This test is different for Dex and MemIndex: while
-// MemIndex manages response deduplication, Dex simply returns all matched
-// symbols which means there might be equivalent symbols in the response.
-// Before drop-in replacement of MemIndex with Dex happens, FileIndex
-// should handle deduplication instead.
 TEST(DexTest, DexDeduplicate) {
   std::vector Symbols = {symbol("1"), symbol("2"), symbol("3"),
  symbol("2") /* duplicate */};
   FuzzyFindRequest Req;
   Req.Query = "2";
   Dex I(Symbols, RefSlab(), URISchemes);
-  EXPECT_FALSE(Req.Limit);
-  EXPECT_THAT(match(I, Req), ElementsAre("2", "2"));
+  EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(DexTest, DexLimitedNumMatches) {
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -29,11 +29,11 @@
 using namespace clang;
 using namespace clang::clangd;
 
-// FIXME: remove this option when Dex is stable enough.
+// FIXME: remove this option when Dex is cheap enough.
 static llvm::cl::opt
 UseDex("use-dex-index",
-   llvm::cl::desc("Use experimental Dex static index."),
-   llvm::cl::init(true), llvm::cl::Hidden);
+   llvm::cl::desc("Use experimental Dex dynamic index."),
+   llvm::cl::init(false), llvm::cl::Hidden);
 
 static llvm::cl::opt CompileCommandsDir(
 "compile-commands-dir",
@@ -286,14 +286,15 @@
   if (!ResourceDir.empty())
 Opts.ResourceDir = ResourceDir;
   Opts.BuildDynamicSymbolIndex =

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

[PATCH] D53288: [clangd] Optionally use dex for the preamble parts of the dynamic index.

3 matches

Site Navigation

Mail list logo

Footer information