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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for sending this early!
Rough interface comments - mostly looks good though!



================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:26
+
+using PostingList = std::vector<size_t>;
+
----------------
we should at least use a type alias for a DocID (maybe an opaque one - not sure for now)


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:39
+public:
+  virtual bool reachedEnd() const = 0;
+  // FIXME(kbobyrev): Should advance() and advanceTo() return size_t peek()
----------------
or have peek return an InvalidID == size_t{-1}?


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:43
+  virtual bool advance() = 0;
+  virtual bool advanceTo(size_t Rank) = 0;
+  virtual size_t peek() const = 0;
----------------
name Rank is confusing. Type should be DocID (param doesn't need a name in the interface)


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:44
+  virtual bool advanceTo(size_t Rank) = 0;
+  virtual size_t peek() const = 0;
+
----------------
could reasonably call this operator*


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:53
+public:
+  DocumentIterator(std::shared_ptr<PostingList> Documents);
+
----------------
is shared_ptr really needed here? Seems like a raw pointer would be fine. I can see that when rebuilding the index we'll have some fun lifetime issues that are sometimes solved with shared_ptr, but I think that would be at the full-index level, not the individual-posting-list level.

(if you want one less indirection, you could have `using PostingListRef = ArrayRef<size_t>`, but it shouldn't matter)


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:59
+  bool advanceTo(size_t Rank) override;
+  size_t getIndex() const;
+  size_t peek() const override;
----------------
what's this for?
I don't think it's worth exposing for testing, should be able to get at the needed cases through the public interface


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:63
+  ///
+  void reset();
+
----------------
how will you use this? it's not in the interface, and if you know the implementation then creating a new iterator is cheap


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:76
+public:
+  AndIterator(const std::vector<std::shared_ptr<QueryIterator>> &Children);
+
----------------
again, unless you have a strong reason, don't use shared_ptr, as it makes it hard to reason about ownership and lifetimes.

These are a tree, unique_ptr should be fine. That way a top-level owned iterator is self-contained.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:83
+
+  const std::vector<std::shared_ptr<QueryIterator>> getChildren() const;
+
----------------
what's this for?


https://reviews.llvm.org/D49546