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