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