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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

The tests //looked// great and comprehensive... but then I took a look at the code and quickly found some missing test coverage (my "Sadly" comment).



================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:39-41
+    if (std::invoke(__comp, std::invoke(__proj, __a), std::invoke(__proj, __b)))
+      return ranges::minmax_result<const _Tp&> {__a, __b};
+    return ranges::minmax_result<const _Tp&> {__b, __a};
----------------
This needs a regression test. For example,
```
auto r = std::ranges::minmax(one, two, [](int, int) { return false; });
assert(r.min == 1);
assert(r.max == 2);
```
Please add similar regression tests for `ranges::min` and `ranges::max` if they don't already exist.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:57-58
+  ranges::minmax_result<range_value_t<_Rp>> operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    auto __iters = __minmax_element::__fn::__go(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+    return ranges::minmax_result<range_value_t<_Rp>> { *__iters.min, *__iters.max };
+  }
----------------
Sadly, I don't think we can do this. `_Rp` is an `input_range`, which means it's single-pass. You can't pass its iterators to `ranges::minmax_result`. You've sidestepped the guardrail by calling into `__go` directly instead of the constrained `minmax_result::operator()` — and that's fine with me — but it means that what you've got here //will// compile but //won't// work.

We should add a regression test for this. Perhaps using a completely custom `input_iterator`. I've thought of two test cases that use only STL facilities, but libc++ has not implemented either `istream_view` or `move_iterator` yet. Also, my `move_iterator` example is possibly wrong. See https://godbolt.org/z/7Kov5e6oe (and I've asked on Slack why libstdc++ barfs on the latter).


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:24
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
----------------



================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:31
+
+namespace ranges {
+template <class _Tp>
----------------
I'd add a blank line after `namespace ranges {`.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:39
+  static _LIBCPP_HIDE_FROM_ABI constexpr
+  minmax_result<_Ip> __go(_Ip __first, _Sp __last, _Comp __comp, _Proj __proj) {
+    auto __project = [&](_Ip& __p) -> decltype(auto) { return std::invoke(__proj, *__p); };
----------------



================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:40
+  minmax_result<_Ip> __go(_Ip __first, _Sp __last, _Comp __comp, _Proj __proj) {
+    auto __project = [&](_Ip& __p) -> decltype(auto) { return std::invoke(__proj, *__p); };
+
----------------
Given how you use this, I think it could profitably be more like
```
auto __less = [&](_Ip& __it, _Ip& __jt) -> bool {
  return std::invoke(__comp, std::invoke(__proj, *__it), std::invoke(__proj, *__jt));
};
```
I'm ambivalent whether the params should be `const _Ip&`. If it's legal, then I'm leaning toward "yes."


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:42
+
+    ranges::minmax_result<_Ip> __result {__first, __first};
+    if (__first == __last || ++__first == __last)
----------------
Style nit.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:69-70
+          __result.min = __i;
+        if (!std::invoke(__comp, __project(__first), __project(__result.max)))
+          __result.max = __first;
+      }
----------------
In my head this gives the wrong answer for `{1,2,4,4}`; `max` in this case should point to the second `4`, not the first `4`. I may have done the mental simulation wrong, though. Please investigate and add a regression test if needed — in fact, I recommend adding a regression test either way!


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:43-46
+struct TotallyOrderedRVal {
+  int i;
+  bool operator<(const NotTotallyOrdered& o) const&& { return i < o.i; }
+};
----------------
This is unused. (If it had been used, `s/NotTotallyOrdered/TotallyOrderedRVal/` on line 45. But as it is, just remove it.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:139-144
+  { // test projection
+    auto proj = [](int& i) { return i == 5 ? -100 : i; };
+    auto ret = std::ranges::minmax(a, std::ranges::less{}, proj);
+    assert(ret.min == 5);
+    assert(ret.max == 9);
+  }
----------------
I'd like to see another test case to verify that the number stored in `ret.max` is actually the element `e`, and not `proj(e)`. Er, actually, it would suffice to have a test case where `decltype(e) != decltype(proj(e))`. Which you actually have on lines 169-176 (the test involving `struct S`). So actually this is fine, and I wouldn't even object if you removed lines 139-144. :)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:176
+    assert(ret.min.i == 1);
+    assert(ret.max.i == 3);
+  }
----------------
Please add
```
ret = std::ranges::minmax(b, b + 3, {}, &S::i);
assert(ret.min.i == 1);
assert(ret.max.i == 3);
```
so we test the iterator-sentinel version with `S` as well.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:41-44
+static_assert(HasMinMaxElement<std::array<int, 0>>);
+static_assert(!HasMinMaxElement<int>);
+static_assert(!HasMinMaxElement<NoLessThanOp>);
+static_assert(!HasMinMaxElement<NotTotallyOrdered>);
----------------
Replace these four lines with your better tests from above. Just replace `MinMax` with `MinMaxElement` in each case, obviously. :)
```
static_assert(HasMinMax<int (&)[10]>);
static_assert(HasMinMax<int (&&)[10]>);
static_assert(!HasMinMax<NoLessThanOp (&)[10]>);
static_assert(!HasMinMax<NotTotallyOrdered (&)[10]>);

static_assert(HasMinMax<std::initializer_list<int>>);
static_assert(!HasMinMax<std::initializer_list<NoLessThanOp>>);
static_assert(!HasMinMax<std::initializer_list<NotTotallyOrdered>>);
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:154
+constexpr void test_immobile() {
+
+  Immobile arr[]{1, 2, 3};
----------------
Remove this blank line. Also consider moving `arr` into each individual block scope (i.e. duplicate it); our style for these little block scopes is //generally// to make them all self-contained, not referencing (or potentially modifying) anything from the outer scope(s).
Ditto line 125.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax_element.pass.cpp:192-195
+  int a[] = {7, 6, 5, 3, 4, 2, 1, 8};
+  test_iterators(a, a + 8);
+  int a2[] = {7, 6, 5, 3, 4, 2, 1, 8};
+  test_range(a2, a2, a2 + 8);
----------------
`test_iterators` and `test_range` are called only once each. This is fine, but then they don't need to take any parameters; you can just define `a` resp. `a2` as a local variable of `test_iterators` resp. `test_range`.
Ditto in `ranges.minmax.pass.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637