[PATCH] D51029: [clangd] Implement LIMIT iterator

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

first few comments, sorry I'll need to come back to this.



================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:90
   virtual DocID peek() const = 0;
   /// Retrieves boosting score. Query tree root should pass Root->peek() to this
   /// function, the parameter is needed to propagate through the tree. Given ID
----------------
Update documentation.

```Informs the iterator that the current document was consumed, and returns its boost.```

The rest of the doc seems a bit to far in the details, it's hard to follow.
Maybe just
```If this iterator has any child iterators that contain the document, consume()
should be called on those and their boosts incorporated.
consume() must *not* be called on children that don't contain the current doc.```


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:96
   /// shouldn't apply any boosting to the consumed item.
   virtual float consume(DocID ID) = 0;
 
----------------
There's two possible formulations here:
 - the DocID parameter is used by the Iterator to determine whether to actually decrement its limit (do we actually have that doc?)
 - the DocID parameter is redundant, as the parent should only call consume() on children that have the doc.
We need to be explicit about which. It seems you want the second (based on offline discussion). In this case we should either assert that the DocID matches peek(), or just drop the DocID parameter and use peek() instead. (If this later means we need to optimize peek, so be it).


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:168
 
+/// Returns LIMIT iterator, which is exhausted as soon requested number of items
+/// is consumed from the root of query tree.
----------------
This comment is accurate, but maybe a bit too concise as what's going on here is a little subtle.
Maybe expand a little like

```
Returns LIMIT iterator, which yields up to N elements of its child iterator.
Elements only count towards the limit if they are part of the final result set.
Therefore the following iterator (AND (2) (LIMIT (1 2) 1)) yields (2), not ().
```


https://reviews.llvm.org/D51029