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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:68
+      return {*__result.min, *__result.max};
+    }
+
----------------
Should the rest of the function go into an `else` branch, to make sure it's part of `if constexpr`?


================
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);
+  }
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:17-23
+// template<forward_iterator I, sentinel_for<I> S, class Proj = identity,
+//   indirect_strict_weak_order<projected<I, Proj>> Comp = ranges::less>
+// constexpr I ranges::minmax(I first, S last, Comp comp = {}, Proj proj = {});
+//
+// template<forward_range R, class Proj = identity,
+//   indirect_strict_weak_order<projected<iterator_t<R>, Proj>> Comp = ranges::less>
+// constexpr borrowed_iterator_t<R> ranges::minmax(R&& r, Comp comp = {}, Proj proj = {});
----------------
This looks incorrect (it looks like the signatures of `ranges::minmax_element` except for the function name).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:40
+};
+struct MoveOnly {
+  MoveOnly(MoveOnly&&) = default;
----------------
Can you also check that something that isn't an `input_range` SFINAEs away?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:195
+
+int main(int, char**) {
+  test();
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637