[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97
+    }
+    // FIXME(kbobyrev): Add True iterator as soon as it's implemented otherwise.
+    // If TopLevelChildren vector will be empty it will trigger an assertion.
----------------
As discussed offline, triggering assertion seems to be a pretty bad behavior. Although the trigram generation, as you suggested, always more than one token, we should try to get rid of this FIXME by introducing the true iterator as proposed here.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:121
+    std::vector<DocID> SymbolDocIDs = consume(*QueryIterator, ItemsToRetrieve);
+    More = SymbolDocIDs.size() >= Req.MaxCandidateCount;
+
----------------
Still, we shouldn't set `More` here.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:58
+private:
+  std::shared_ptr<std::vector<const Symbol *>> Symbols;
+  llvm::DenseMap<SymbolID, const Symbol *> LookupTable;
----------------
nit: add /*GUARDED_BY(Mutex)*/


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:366
+
+TEST(DexIndex, FuzzyFind) {
+  DexIndex Index;
----------------
Please add tests with empty `Query`.


https://reviews.llvm.org/D50337