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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
