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

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

Quuxplusone 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));
+    };
----------------
philnik wrote:
> 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.
This is detectable by the user's projection, so yes, please forward them. I think a detector would look like
```
int a[2] = {1, 2};
auto r = std::ranges::subrange(std::move_iterator(a), std::move_iterator(a + 2));
auto proj = [](int&&) { return 42; };  // callable only with rvalues
auto [min, max] = std::ranges::minmax(r, {}, proj);
assert(min == 1 && max == 2);
```
Also, what's probably muddying the issue here is that you named them `__it1` and `__it2` but they are not in fact iterators; they're elements/values! Please change the names to something like `__a` and `__b`. (And in general, please don't name non-iterators `it`. Arguably it could be short for "item," but in practice, no, it's just confusing.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:226
+
+  { // check that the iterator isn't moved from multiple times
+    std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637