[PATCH] D50337: [clangd] DexIndex implementation prototype
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 17 08:18:01 PDT 2018
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
More information about the cfe-commits
mailing list