[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:109
+  float consume() override {
+    if (reachedEnd())
       return DEFAULT_BOOST_SCORE;
----------------
this isn't possible, as the item being consumed is the value of peek().
You could assert !reachedEnd() or just delete this.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:227
+  float consume() override {
+    if (reachedEnd())
       return DEFAULT_BOOST_SCORE;
----------------
and again here


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:341
+  float consume() override {
+    if (!reachedEnd())
+      --ItemsLeft;
----------------
if condition is always true
you could replace the if with an assertion, but the child will assert, so better just to remove it.
replace if with assertion


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:348
+  llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+    OS << "(LIMIT items left " << ItemsLeft << *Child << ")";
+    return OS;
----------------
ISTM this should show the immutable state at the start of the query, rather than the mutated state?
Or if you really think it's useful, both.

e.g. (LIMIT 5(3) <child>)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:358
 
 std::vector<std::pair<DocID, float>> consume(Iterator &It, size_t Limit) {
   std::vector<std::pair<DocID, float>> Result;
----------------
I think you can remove the Limit parameter now, replacing it with an outer Limit iterator


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:362
        It.advance(), ++Retrieved) {
     DocID Document = It.peek();
+    Result.push_back(std::make_pair(Document, It.consume()));
----------------
you can inline this now


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:267
 
 TEST(DexIndexIterators, Limit) {
+  const PostingList L0 = {3, 6, 7, 20, 42, 100};
----------------
Is this testing the "limit" parameter to consume, or the limit iterator?
It shouldn't test both just because they happen to share a name :-)

(As noted above, I think you can remove the limit parameter)


https://reviews.llvm.org/D51029