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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
