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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A few more comments about the bits I understand, but waiting mostly on the documentation.



================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:155
+  llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+    OS << "(&& ";
+    for (size_t Index = 0; Index < Children.size(); ++Index) {
----------------
nit: `&` should be enough?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:175
+// FIXME(kbobyrev): This compares IDs of two iterators...
+auto CompareIterators = [](std::unique_ptr<QueryIterator> &Left,
+                           std::unique_ptr<QueryIterator> &Right) -> bool {
----------------
can this just be a static function?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:196
+        });
+    std::make_heap(begin(Children), end(Children), CompareIterators);
+  }
----------------
can we start with the simple/obvious implementation, and add heap later if it actually improves performance?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:257
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                              const QueryIterator &Iterator) {
----------------
you can just inline this.

If you don't, you have to provide a declaration in the header, outside the class definition. (In addition to the friend declaration). GCC errors on this, clang ignores it (GCC is right).


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:28
+
+using DocID = size_t;
+using PostingList = std::vector<DocID>;
----------------
`uint32_t` should be plenty. These will be a large fraction of the index's memory usage. (Especially if we push actual payloads onto disk later)


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:41
+// FIXME(kbobyrev): Document this one properly.
+// FIXME(kbobyrev): Don't expose all the classes in the header, move classes to
+// header and only have constructors here.
----------------
this is done I think.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:43
+// header and only have constructors here.
+class QueryIterator {
+public:
----------------
I'm not sure `Query` actually adds much to this class name. `Iterator`, `AndIterator`, etc would read more smoothly.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:47
+  // instead?
+  virtual bool reachedEnd() const = 0;
+  virtual bool advance() = 0;
----------------
nit: this seems non-orthogonal, could we either:
 - have `advance()` and `advanceTo()` return void
 - or remove `reachedEnd()` and have owners that need to keep the state (and/or iterators) maintain it separately?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:52
+  virtual DocID peek() const = 0;
+  virtual size_t cost() const = 0;
+
----------------
As discussed offline, please leave this out of the initial patch.
It requires quite some explanation and understanding, and seems likely to delay the review.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:54
+
+  virtual llvm::raw_ostream &dump(llvm::raw_ostream &OS) const = 0;
+
----------------
private?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:68
+std::unique_ptr<QueryIterator>
+constructAndIterator(std::vector<std::unique_ptr<QueryIterator>> &&Children);
+
----------------
nit: `newAndIterator` or just `andIterator`? brevity is the soul of wit and all that

Or even better, what about:

```
Iterator::createAnd(...)
Iterator::createOr(...)
Iterator::create(PostingList*)
```


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:68
+std::unique_ptr<QueryIterator>
+constructAndIterator(std::vector<std::unique_ptr<QueryIterator>> &&Children);
+
----------------
sammccall wrote:
> nit: `newAndIterator` or just `andIterator`? brevity is the soul of wit and all that
> 
> Or even better, what about:
> 
> ```
> Iterator::createAnd(...)
> Iterator::createOr(...)
> Iterator::create(PostingList*)
> ```
nit: pass by value rather than by rvalue-reference unless there's a strong reason.

Here, vector<u_p> isn't copyable, and there are no overloads, and this isn't performance-critical, so it should be fine.


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:19
+
+using std::make_shared;
+using std::max;
----------------
please spell these out (local style)


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:27
+
+using llvm::raw_string_ostream;
+
----------------
using namespace llvm


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:67
+
+  vector<unique_ptr<QueryIterator>> Children;
+
----------------
nit: just `auto And = constructAndIterator({constructDocumentIterator(EmptyList)})`

(and below in various places).
This actually seems nicer - the expression hierarchy reflects the iterator hierarchy.


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:75
+
+  string Dump;
+  raw_string_ostream OS(Dump);
----------------
just `EXPECT_EQ(to_string(*And), "(&& [])")`

You may need to `#include "llvm/Support/ScopedPrinters.h"`


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:98
+
+  EXPECT_EQ(And->reachedEnd(), false);
+  EXPECT_EQ(And->peek(), 0U);
----------------
add a helper function `vector<DocID> consume(Iterator&)`
(Maybe even to the main file rather than the tests).

Then this section (and some others) can just be `EXPECT_THAT(consume(*And), ElementsAre(0u,7u,10u,...))`


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:140
+
+  // More complicated example: nested And iterators.
+  //
----------------
Hmm, I'm not sure this approach scales - there shouldn't be anything special about composing and/and, compared to and/or, or/not, boost/and, etc.

I'd suggest testing each iterator directly, and then having one test that uses a more complex tree, and dropping this case.


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
+TEST(DexIndexIterators, QueryTree) {
+  // An example of more complicated query
+  //
----------------
This is too complicated. Try to use each iterator once or twice.
(We'll need to extend this when we add new iterator types)


https://reviews.llvm.org/D49546