[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks really nice. 
Iterator implementations can be simplified a bit I think.



================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:50
+      OS << Documents[Index];
+      if (Index + 1 != Documents.size())
+        OS << ", ";
----------------
(uber-nit: slightly clearer to add the separator first, then you only have to compare against zero.
Or this idiom:
```const char* Sep = "";
for (DocID D : Documents) {
  OS << Sep << D;
  Sep = ", ";
}
```


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:59
+  PostingListRef Documents;
+  DocID Index;
+};
----------------
nit: the code might be slightly more concise (particularly advanceTo) if the current element is represented by a PostingListRef::Iterator rather than an index, up to you.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:59
+  PostingListRef Documents;
+  DocID Index;
+};
----------------
sammccall wrote:
> nit: the code might be slightly more concise (particularly advanceTo) if the current element is represented by a PostingListRef::Iterator rather than an index, up to you.
(DocID is the wrong type for an index)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:65
+public:
+  AndIterator(std::vector<std::unique_ptr<Iterator>> &&AllChildren)
+      : Children(std::move(AllChildren)), ReachedEnd(false) {
----------------
nit: just take children by value without &&


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:69
+    DocID ID = 0;
+    for (auto &Child : Children) {
+      // Check whether any child iterator points to the end. In this case,
----------------
nit: const auto &


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:74
+        ReachedEnd = true;
+        break;
+      }
----------------
just return so you don't need the if at the end?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:88
+  // FIXME(kbobyrev): Properly document this one.
+  void advance() override {
+    assert(!reachedEnd() && "AndIterator can't call advance() at the end.");
----------------
It would be nice if the functions in this class could share some logic.

what about:

```
for (auto& C : children)
  C->advance();
sync();
```

where `sync` is
```
// restores class invariants: ReachedEnd set, or all children in sync
void sync() {
DocID Max = 0;
for (auto &C : Children) {
   if (C->reachedEnd()) {
    ReachedEnd = true;
    return;
  }
  Max = std::max(C->peek(), Max);
}
for (auto &C : Children) {
  C->advanceTo(Max);
   if (C->reachedEnd()) {
    ReachedEnd = true;
    return;
  }
}
}
```

the trick is then the constructor is just `sync();`, `advanceTo` is just "advance all children and sync", etc.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:155
+// FIXME(kbobyrev): Document this one properly.
+// FIXME(kbobyrev): Currently, the implementation is inefficient because peek()
+// and reachedEnd() are computed in O(Children.size()). This can be fixed by
----------------
I think this is `FIXME: would a min-heap be faster?` :-)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:181
+    const auto HighestID = peek();
+    DocID ChildrenToAdvance = 0;
+    while ((ChildrenToAdvance++ < Children.size()) && !reachedEnd() &&
----------------
I can't follow this one - why is the outer loop needed?

(It looks like you're using DocID as integer again, but I can't tell why the variable is needed at all)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:184
+           (peek() == HighestID)) {
+      for (auto &Child : Children) {
+        if (!Child->reachedEnd() && Child->peek() == HighestID) {
----------------
nit: you can drop the extra braces here and in advanceTo (at least, you use that style above)


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:204
+           "OrIterator must have at least one child to peek().");
+    bool FoundFirstValid = false;
+    DocID Result = 0;
----------------
why not just initialize Result to `std::numeric_limits<DocID>::max()`, and skip the valid checking?

You know by assertion that you'll find something better.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:235
+  std::vector<DocID> Result;
+  while (!It.reachedEnd()) {
+    Result.push_back(It.peek());
----------------
`for (; !It.reachedEnd(); It.advance())`


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:50
+
+  static std::vector<DocID> consume(Iterator &It);
+
----------------
this doesn't really make sense as static as it operates on an Iterator.
Either a free function or non-static member seems fine


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:65
+  static std::unique_ptr<Iterator>
+  createAnd(std::vector<std::unique_ptr<Iterator>> Children);
+
----------------
nit nit: move above the template overload so you can document the readable one.


https://reviews.llvm.org/D49546