[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

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

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

The headline here is that we need to remember to `std::invoke` our predicates and projections. Sorry I'm just catching this now.



================
Comment at: libcxx/include/__algorithm/ranges_min.h:16
+#include <__functional/ranges_operations.h>
+#include <iterator>
+
----------------
Granularize?


================
Comment at: libcxx/include/__algorithm/ranges_min.h:34
+  const _Tp& operator()(const _Tp& __a, const _Tp& __b, _Comp __comp = {}, _Proj __proj = {}) const {
+    return __comp(__proj(__a), __proj(__b)) ? __a : __b;
+  }
----------------
Argh! Peeking at range-v3's code has just reminded me that we need to be using `std::invoke` for projections //and// comparators. Please fix here and probably in some earlier reviews too (sadly).
The test case for this would be, like,
```
struct S { int i; };
S a[3] = { S{2}, S{1}, S{3} };
decltype(auto) ret = std::ranges::min(a[0], a[1], {}, &S::i);
ASSERT_SAME_TYPE(decltype(ret), const S&);
assert(&ret == &a[1]);
assert(ret.i == 1);
```
and likewise for the initializer_list and range overloads.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:41
+  _Tp operator()(initializer_list<_Tp> __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    return *ranges::__min_element::__fn::__go(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+  }
----------------
This works, but why not just `return *ranges::min_element(ranges::begin(__r), ranges::end(__r)......` oh, because you don't want to copy the comparator or the projection, right!
Very clever; I guess I have no problem with this. But attn @ldionne, do you think this is OK and/or can you think of a better pattern? Because I imagine we'll be doing this a lot.

AFAICT, range-v3 takes the middle path: it doesn't unnecessarily //copy// callables (much?), but it also takes no special precautions to avoid //move-constructing// them. We could go that route, but I think avoiding unnecessary moves is a QoI thing and we might as well do it right the first time.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:43-44
+static_assert(!HasMin<int>);
+static_assert(!HasMin<NoLessThanOp>);
+static_assert(!HasMin<NotTotallyOrdered>);
+
----------------
You probably intended
```
static_assert( HasMin<int(&)[10]>);
static_assert(!HasMin<NoLessThanOp(&)[10]>);
static_assert(!HasMin<NotTotallyOrdered(&)[10]>);
```
and/or
```
static_assert(!HasMin<std::initializer_list<NoLessThanOp>>);
static_assert(!HasMin<std::initializer_list<NotTotallyOrdered>>);
```
FYI, I'm getting more and more uncomfortable with this `HasXxx` pattern, because it makes it impossible to test rvalue types. I think you should switch, at least in new code, to
```
static_assert(std::invocable<decltype(std::ranges::min), std::initializer_list<int>>);
static_assert(std::invocable<decltype(std::ranges::min), int(&)[10]>);
static_assert(std::invocable<decltype(std::ranges::min), int(&&)[10]>); // impossible to express right now
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:52
+  // test projection
+  assert(std::ranges::min(1, 2, std::ranges::less{}, [](int i){ return i == 1 ? 10 : i; }) == 2);
+}
----------------
I'd like a test for `decltype(ranges::min(1,2))`, etc. — it should return a `const int&`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:75
+    };
+    [[maybe_unused]] std::same_as<int> auto ret = std::ranges::min({1, 2, 3}, comparator, projection);
+    assert(compares == 2);
----------------
This should be `std::same_as<int> decltype(auto)` if that's allowed (I think it probably isn't), or else write it out longhand with `decltype(auto) ret = ~~~ ASSERT_SAME_TYPE(decltype(ret), ~~~)`. What we're worried about here is that this overload of `ranges::min` should return exactly `int`, not a `const int&` that might dangle.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:84
+  { // test projection
+    int ret = std::ranges::min(a, std::ranges::less{}, [](int i) { return i == 5 ? -100 : i; });
+    assert(ret == 5);
----------------
Nit: consider `auto projection = []...` to shorten this line.
Coverage: As recently discussed with @var-const in the `std::mergeable` PR, I'd like to see a little coverage for a projection like `[](int& i) { ... }`. In other words, `std::ranges::min(a)` should be forwarding along the constness of its range's elements. Notice that the elements of an `initializer_list<T>` are //already// `const T`; that's why the made-up iterator type in those overloads' constraints is `const T*` instead of `T*`. But when the range is a plain old non-const array, we should be able successfully to treat its elements as non-const lvalues.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:102
+    };
+    [[maybe_unused]] std::same_as<int> auto ret = std::ranges::min(std::array{1, 2, 3}, comparator, projection);
+    assert(compares == 2);
----------------
Remember that `[[maybe_unused]]` is a code smell!
You should be asserting that `ret == 1` here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:106
+  }
+  { // test borrowed range
+    int ret = std::ranges::min(std::views::all(a));
----------------
`views::all` isn't //just// borrowed, but also a view; and I don't think either of those properties are "interesting" to `ranges::min`, are they? This test case might be redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589