[PATCH] D51310: [clangd] Implement iterator cost

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:93
+                 const std::unique_ptr<Iterator> &RHS) {
+                return LHS->cost() < RHS->cost();
+              });
----------------
sammccall wrote:
> (I'd actually suggest declaring operator< in iterator.h and making it compare by cost, it seems natural/important enough, and saves duplicating this lambda)
Actually, I think sorting the children of OR iterator doesn't improve performance so I should probably just have a lambda in one place (`AND`) as it won't be used anywhere else.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:125
+  size_t cost() override {
+    return std::accumulate(
+        begin(Children), end(Children), Children.front()->cost(),
----------------
sammccall wrote:
> sammccall wrote:
> > The use of `std::accumulate` here seems less clear than the direct:
> > ```size_t Smallest = ...;
> > for (const auto& Child : Children)
> >   Smallest = std::min(Smallest, Child->cost());
> > return Smallest;
> > ```
> > For better or worse, functional styles don't have the most natural syntax in C++, and (IMO) shouldn't be used just because they fit.
> > 
> > (This is consistent with other places, but I also think that the same applies there)
> actually, we've sorted by cost, so don't we just want Children.front().cost() here?
It's a sad day for the noble λ knights, but I agree.

I think I should push it like this in this patch (to comply with the common style) and then send out a separate patch with refactorings (I had few concerns about the inconsistency in the `Iterator.cpp`).


https://reviews.llvm.org/D51310