[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:68
+      return {*__result.min, *__result.max};
+    }
+
----------------
var-const wrote:
> Should the rest of the function go into an `else` branch, to make sure it's part of `if constexpr`?
I'm not a huge fan of that, but I agree that having faster compile times is more valuable than the bit of increased readability it gives to not have the else branch. Just for the record, I would definitely not want the else branch if it were a normal if.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:62
+    auto __less = [&](auto&& __it1, auto&& __it2) -> bool {
+      return std::invoke(__comp, std::invoke(__proj, __it1), std::invoke(__proj, __it2));
+    };
----------------
Quuxplusone wrote:
> philnik wrote:
> > var-const wrote:
> > > Do `__it1` and `__it2` need to be forwarded, given that they are forwarding references?
> > I wasn't able to produce any case where it makes a difference. The main reason these are forwarding is that `iterator::operator*` could return an rvalue reference. So I essentially just cast them to lvalues. There is also no guarantee that the calls will be made in a specific way.
> This is detectable by the user's projection, so yes, please forward them. I think a detector would look like
> ```
> int a[2] = {1, 2};
> auto r = std::ranges::subrange(std::move_iterator(a), std::move_iterator(a + 2));
> auto proj = [](int&&) { return 42; };  // callable only with rvalues
> auto [min, max] = std::ranges::minmax(r, {}, proj);
> assert(min == 1 && max == 2);
> ```
> Also, what's probably muddying the issue here is that you named them `__it1` and `__it2` but they are not in fact iterators; they're elements/values! Please change the names to something like `__a` and `__b`. (And in general, please don't name non-iterators `it`. Arguably it could be short for "item," but in practice, no, it's just confusing.)
I'm not adding a regression test because I think it isn't actually detectable in any sensible way. The projection has to be callable with an lvalue, because it may have the temporary as its argument.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:84
+  ranges::minmax_result<_Ip> operator()(_Ip __first, _Sp __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    return __go(__first, __last, __comp, __proj);
+  }
----------------
Quuxplusone wrote:
> var-const wrote:
> > philnik wrote:
> > > var-const wrote:
> > > > Would it make sense to move `__first` and maybe `__last`?
> > > iterators should //always// be cheap to copy, and `ranges::minmax_element` doesn't support input ranges. So I see no reason to move them.
> > I don't feel very strongly, but I don't think we should rely on users doing something that isn't enforced. Elsewhere we're protecting against things that are pretty unreasonable just because they're allowed. It's a good point that an iterator that's expensive to copy is more likely to be an input iterator, though.
> FWIW, if we really thought iterators/sentinels might be //expensive//, we might even choose to pass them by reference instead of by value. (In this specific case, we could take `const _Sp&`.) We're just guessing that `_Sp` is likely more appropriate than `const _Sp&` (because //usually// `_Sp` is either a pointer/iterator, or a tag type). I would //not// pass-by-reference just to avoid a copy of an iterator or sentinel. (But I would pass-by-reference to avoid copies of callables. Inconsistent, yes. :))
> 
> Also, the big downside of adding std::moves has already been illustrated, by me, literally yesterday: in D118003 I wrote a std::move that shouldn't have been there, and thus introduced a use-after-move bug. We have no good story for testing against use-after-move of iterators and sentinels. So let's be cautious in this department.
> 
> However, just changing `__first` into `std::move(__first)` sounds like a pretty harmless improvement to me. It //might// be faster and it will //never// be slower. So +1 on std::move'ing from me.
> I do see how it's a slippery slope, because our STL Classic algorithms generally do //not// std::move iterators, and I don't think we should actually go churn up that existing code to add moves. But in new code, and specifically Ranges code where we might expect slightly-heavier-weight iterators, and //specifically// in this case where the function is one line long — I think we can justify std::move!
> 
> (But I'll defer to @ldionne if he has an opinion.)
Agreed that it doesn't hurt here, but I definitely wouldn't make that a guarantee. 


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:195
+
+int main(int, char**) {
+  test();
----------------
var-const wrote:
> philnik wrote:
> > var-const wrote:
> > > I'd add a few test cases to make sure the algorithm works correctly:
> > > - make sure to test both an even- and an odd-length range (since the algorithm effectively iterates in steps of two);
> > > - empty range;
> > > - single-element range;
> > > - sorted range (both ascending and descending);
> > > - all elements compare equal (this should also test that "Returns `X{x, y}`, where `x` is a copy of the //leftmost// element with the smallest value and `y` a copy of the //rightmost// element with the largest value in the input range").
> > > 
> > > This applies to `minmax_element` as well.
> > > 
> > Even and odd lengths are already covered. In `ranges::minmax` empty ranges aren't allowed. I added asserts. What are you looking for with a sorted range?
> Re. sorted range -- it seems like an interesting case because min or max would be updated on every iteration.
> 
> I don't see a test for a single-element range (except the test with `shared_ptr`, but it's focused on a different thing).
I think I got all you requests covered now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120637/new/

https://reviews.llvm.org/D120637