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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Upcoming changes:

- Improve debugging experience by providing `llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, std::unique_ptr<QueryIterator> Iterator` to recursively pretty print queries in human-readable format: e.g. `(& [0, 1, 2, 3] ([3, 4, 6, 8] || [0, 1]))`. This can be later used for debugging and making sure optimizations were correct.
- Properly document the interface and implementation. The current documentation is just a draft, it should be refined.
- Think about introducing iterator `cost` and applying cheap optimizations like pre-sorting children of `AndIterator` before performing `advance()`: this way the iterator doesn't spend too much time iterating through long posting lists when it has are short/empty children



================
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()
----------------
sammccall wrote:
> or have peek return an InvalidID == size_t{-1}?
Tried to do that, but that creates additional complexity for the `AndIterator` which stores `ReachedEnd` anyway. Otherwise I would either have to spend `O(Children.size())` to understand that `AndIterator` reached the end or have inconsistent APIs across the classes, I think it's better to leave it as it is.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:44
+  virtual bool advanceTo(size_t Rank) = 0;
+  virtual size_t peek() const = 0;
+
----------------
sammccall wrote:
> could reasonably call this operator*
As discussed offline, `*(*Child)` would be probably not very clean.


https://reviews.llvm.org/D49546