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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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 {
----------------
sammccall wrote:
> can this just be a static function?
Wiped this one. Also, in the future I guess it's better to implement `operator<()` for iterators, because that's what this essentially is.


================
Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.cpp:196
+        });
+    std::make_heap(begin(Children), end(Children), CompareIterators);
+  }
----------------
sammccall wrote:
> can we start with the simple/obvious implementation, and add heap later if it actually improves performance?
Done. It doesn't look simpler to me, but I tried to keep it as simple as possible. I also put a `FIXME` on top of `OrIterator` with the trade-offs description.


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


https://reviews.llvm.org/D49546