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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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);
----------------
philnik wrote:
> 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&`.
I'm also not 100% sure I know that ranges does some "interesting" things with `&&`. Maybe @var-const or @ldionne knows for sure.


================
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]);
----------------
philnik wrote:
> 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.
Based on [expr.add].4 (http://eel.is/c++draft/expr.add#4), my conclusion is that 4.3 applies and the behaviour is undefined.
But it's odd that Clang and MSVC accept it. Still I'm not convinced it's not a compiler bug since GCC accepts every value, including `42` and `-42`.
(Changing `int i;` to `int i[1];` makes the code valid.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122002