[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Almost LG! Just a few more nits.



================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:87
+  std::vector<DocID> SymbolDocIDs;
+  std::priority_queue<std::pair<float, const Symbol *>> Top;
+
----------------
nit: move `SymbolDocIDs` and `Top` closer to where they're used.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97
+    // Add OR iterator for scopes if the request contains scopes.
+    if (!Req.Scopes.empty()) {
+      TopLevelChildren.push_back(createScopeIterator(Req.Scopes));
----------------
I think we should let `createScopeIterator` handle empty scope list case; it can return an empty list anyway.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:109
+    SymbolDocIDs = consume(*QueryIterator, ItemsToRetrieve);
+    More = SymbolDocIDs.size() >= Req.MaxCandidateCount;
+
----------------
This is not a proper place to set `More`. It's already handled below.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:112
+    // Populate scores.
+    for (size_t SymbolIdx = 0; SymbolIdx < SymbolDocIDs.size(); ++SymbolIdx) {
+      const auto Sym = (*Symbols)[SymbolDocIDs[SymbolIdx]];
----------------
nit: use range-based for loop?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:113
+    for (size_t SymbolIdx = 0; SymbolIdx < SymbolDocIDs.size(); ++SymbolIdx) {
+      const auto Sym = (*Symbols)[SymbolDocIDs[SymbolIdx]];
+      const auto Score = Filter.match(Sym->Name);
----------------
nit: Maybe take `const auto &Sym`?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:119
+      // highest actual score.
+      Top.emplace(-*Score * SymbolQuality.find(Sym)->second, Sym);
+      if (Top.size() > Req.MaxCandidateCount) {
----------------
nit: `- (*Score) * ...` for readability.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:127
+    // Apply callback to the top Req.MaxCandidateCount items.
+    for (; !Top.empty(); Top.pop())
+      Callback(*Top.top().second);
----------------
Can we simply iterate without `pop()`?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:150
+std::unique_ptr<Iterator>
+DexIndex::createTrigramIterator(std::vector<Token> TrigramTokens) const {
+  std::vector<std::unique_ptr<Iterator>> TrigramIterators;
----------------
This assumes that `createTrigramIterator` and `createScopeIterator` are already guarded by the mutex, which is implicit. I think we can make it clearer by making these local helpers that take in InvertedIndex` with the requirement that local has been acquired.


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:366
+
+// FIXME(kbobyrev): Add more tests on DexIndex? Right now, it's mostly a wrapper
+// around DexIndex, adopting tests from IndexTests.cpp sounds reasonable.
----------------
What tests do we want? If it's related to the changes in this patch, we should add it now. Tests shouldn't be `FIXME` :)


https://reviews.llvm.org/D50337