[PATCH] D52083: [clangd] Store OR iterator children in heap

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:128
 
+// Return LHS > RHS.
+auto Compare = [](const std::unique_ptr<Iterator> &LHS,
----------------
NIT: use triple-slash comments.
NIT: LHS > RHS seems to be exactly what's defined by this function. Maybe mention `peek()` to explain how actual comparison works?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:129
+// Return LHS > RHS.
+auto Compare = [](const std::unique_ptr<Iterator> &LHS,
+                  const std::unique_ptr<Iterator> &RHS) {
----------------
Turn lambda into a function?


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:129
+// Return LHS > RHS.
+auto Compare = [](const std::unique_ptr<Iterator> &LHS,
+                  const std::unique_ptr<Iterator> &RHS) {
----------------
ilya-biryukov wrote:
> Turn lambda into a function?
Call `Greater` to avoid confusion? `Compare` can mean comparisons in both directions.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:197
+    for (const auto &Child : Children) {
+      if (Child->peek() != ID)
+        break;
----------------
This seems to assume Children is sorted, but that's not the case.
Why is it valid to iterate only a subset of the vector?


https://reviews.llvm.org/D52083