[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:62
+    auto __less = [&](auto&& __it1, auto&& __it2) -> bool {
+      return std::invoke(__comp, std::invoke(__proj, __it1), std::invoke(__proj, __it2));
+    };
----------------
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.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:70
+    if (__less(*__first, __result.min))
+      __result.min = *__first;
+    else
----------------
var-const wrote:
> I have a concern similar to `mismatch` -- this could be a lot of copying in the worst case. Would it be worthwhile to have different implementations of the algorithm for `input_range` and `forward_range`?
I've also added two moves for the last uses of `__i`.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:76-81
+      if (++__first == __last) {
+        if (__less(__i, __result.min))
+          __result.min = __i;
+        else if (!__less(__i, __result.max))
+          __result.max = __i;
+        return __result;
----------------
var-const wrote:
> Same optional suggestion re. using lambdas as in `minmax_element`.
With the moves every branch is different anyways, so I don't think it makes much sense.


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


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:91
+  ranges::minmax_result<borrowed_iterator_t<_Rp>> operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    return __go(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+  }
----------------
var-const wrote:
> Question: does `__r` need to be forwarded, perhaps?
Definitely not the first one, and I also don't think the second one. We still need our range for the iterators to be valid, so conceptually it doesn't make sense to forward them (and makes probably no practical difference).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637