[PATCH] D50337: [clangd] DexIndex implementation prototype
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 16 07:54:33 PDT 2018
ioeric added inline comments.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:25
+ std::vector<std::pair<float, const Symbol *>> &Scores) {
+ // First, sort retrieved symbols by the cumulative score to apply callbacks
+ // in the order of descending score.
----------------
Why is this enforced? `fuzzyFind` doesn't say anything about callback order.
Also `useCallback` seems to be the right abstraction to me; different requests can have different callback behaviors. I think we could simply inline the code here.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:99
+ std::vector<DocID> SymbolDocIDs;
+ // FIXME(kbobyrev): Scoring all matched symbols and then processing
+ // MaxCandidateCount items is rather expensive, this should be replaced by
----------------
Any reason we are not doing the two-stage scoring now? Retrieving while scoring with more expensive scoring seems to be diverging from the expected design. I think we can retrieve a relatively large number of symbols (e.g. a fixed large number or `100*MaxCandidateCount`?) before re-scoring them with more expensive scorers (e.g. fuzzy match), as consuming only `Req.MaxCandidateCount` symbols from the iterator tree can easily leave out good candidates (e.g. those that would've gotten good fuzzy match scores).
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:110
+ // trigrams.
+ const auto TrigramTokens = generateIdentifierTrigrams(Req.Query);
+ std::vector<std::unique_ptr<Iterator>> TrigramIterators;
----------------
It seems that the trigram generation could be done outside of the critical section?
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:111
+ const auto TrigramTokens = generateIdentifierTrigrams(Req.Query);
+ std::vector<std::unique_ptr<Iterator>> TrigramIterators;
+ for (const auto &Trigram : TrigramTokens) {
----------------
I think we could pull some helper functions here to make the code a bit easier to follow e.g. `createTrigramIterators(TrigramTokens)`, `createScopeIterators(scopes)`...
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:50
+
+ virtual void
+ lookup(const LookupRequest &Req,
----------------
Why virtual?
================
Comment at: clang-tools-extra/unittests/clangd/TestIndexOperations.h:1
+//===-- IndexHelpers.h ------------------------------------------*- C++ -*-===//
+//
----------------
As this file contains helper functions for testing indexes, I'd suggest calling this `TestIndex.h` like `TestTU.h`.
https://reviews.llvm.org/D50337
More information about the cfe-commits
mailing list