[libcxx-commits] [PATCH] D122002: [libc++][ranges] Implement ranges::max

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_max.h:51
+    _LIBCPP_ASSERT(__il.begin() != __il.end(), "initializer_list must contain at least one element");
+    auto __comp2 = [&](auto&& __lhs, auto&& __rhs) { return std::invoke(__comp, __rhs, __lhs); };
+    return *ranges::__min_element_impl(__il.begin(), __il.end(), __comp2, __proj);
----------------
Mordante wrote:
> Mordante wrote:
> > I don't like the name `__comp2` has no meaning. How about `__comp_lhs_rhs_swapped`.
> > That name give a hint what's about to happen and it makes easier to understand why `__min_element_impl` is used.
> Why are you using `auto&&` without `std::forward`?
It's not 100% clear to me if cv qualifications have to stay. I'm pretty sure that `__comp` has to be callable with lvalues. If cv-qualifications don't have to be kept, the I can use `const auto&`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max.pass.cpp:97
     int i;
     int* a[] = {&i, &i + 1};
+    auto ret = std::ranges::max(a[0], a[1]);
----------------
Mordante wrote:
> I'm not convinced this is valid. Taking the address of one beyond `i` when `i` isn't an array.
> Are you sure this is valid? If not we should fix this also in the original min test.
https://compiler-explorer.com/z/rTTce8YKh At least clang and MSVC only complain if I try to get a two-past-the-end pointer. GCC doesn't complain for some reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122002