[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: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