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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

@philnik Please update `RangesAlgorithms.csv` as well. Thanks!



================
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));
+    };
----------------
Do `__it1` and `__it2` need to be forwarded, given that they are forwarding references?


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:70
+    if (__less(*__first, __result.min))
+      __result.min = *__first;
+    else
----------------
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`?


================
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;
----------------
Same optional suggestion re. using lambdas as in `minmax_element`.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:57
+      if (++__first == __last) {
+        if (__less(__i, __result.min))
+          __result.min = __i;
----------------
Very optional: I found several nested conditions with similar comparisons a little hard to keep track off. Consider streamlining some of this flow using a helper -- maybe something like:
```
    auto __less = [&](_Ip& __it1, _Ip& __it2) -> bool {
      return std::invoke(__comp, std::invoke(__proj, *__it1), std::invoke(__proj, *__it2));
    };
    auto __maybe_update_min = [&__result](_Ip& __it) {
      if (__less(__it, __result.min)) {
        __result.min = __it;
      }
    };
    auto __maybe_update_max = [&__result](_Ip& __it) {
      if (!__less(__it, __result.max)) {
        __result.max = __it;
      }
    };

    ranges::minmax_result<_Ip> __result {__first, __first};
    if (__first == __last || ++__first == __last)
      return __result;

    if (__less(__first, __result.min))
      __result.min = __first;
    else
      __result.max = __first;

    while (++__first != __last) {
      _Ip __i = __first;
      if (++__first == __last) {
        __maybe_update_min(__i);
        __maybe_update_max(__i);
        return __result;
      }

      if (__less(__first, __i)) {
        __maybe_update_min(__first);
        __maybe_update_max(__i);
      } else {
        __maybe_update_min(__i);
        __maybe_update_max(__first);
      }
    }
```


================
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);
+  }
----------------
Would it make sense to move `__first` and maybe `__last`?


================
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);
+  }
----------------
Question: does `__r` need to be forwarded, perhaps?


================
Comment at: libcxx/include/algorithm:766
 #include <__algorithm/ranges_min_element.h>
+#include <__algorithm/ranges_minmax.h>
+#include <__algorithm/ranges_minmax_element.h>
----------------
You probably also need to update the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:28
+#include <cassert>
+#include <random>
+#include <ranges>
----------------
`<random>` is unused, right? (here and in the other test file)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:31
+
+#include "test_macros.h"
+#include "test_iterators.h"
----------------
Is `"test_macros.h"` used?


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



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:199
+
+  {
+    std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
----------------
Can you add a comment explaining the main intention of this test case? It seems to be to test `move_iterator` behavior, but then it uses `shared_ptr` as well.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:41
+
+static_assert(HasMinMaxElement<int (&)[10]>);
+static_assert(HasMinMaxElement<int (&&)[10]>);
----------------
Similar to other patches: I think there needs to be a SFINAE test for every constraint (in both test files).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:216
+
+constexpr bool test() {
+
----------------
Nit: seems like an extra blank line.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:220
+  test<bidirectional_iterator<const int*>>();
+  test<random_access_iterator<const int*>>();
+  test<const int*>();
----------------
Optional: also test with a contiguous iterator for good measure (echoing Arthur's comment from another patch)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637