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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but several small issues.



================
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);
----------------
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.


================
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:
> 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`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max.pass.cpp:38
 template <class T>
-concept HasMinR = requires { std::ranges::min(std::declval<T>()); };
+concept HasMinR = requires { std::ranges::max(std::declval<T>()); };
 
----------------
There are other occurrences of `Min` in this file.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max.pass.cpp:91
     ASSERT_SAME_TYPE(decltype(ret), const S&);
-    assert(&ret == &a[1]);
-    assert(ret.i == 1);
+    assert(&ret == a + 0);
+    assert(ret.i == 2);
----------------
IMO This is clearer.


================
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]);
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:12
 // UNSUPPORTED: c++03, c++11, c++14, c++17
-// UNSUPPORTED: libcpp-no-concepts
 // UNSUPPORTED: libcpp-has-no-incomplete-ranges
----------------
I noticed this line on other files too, it will be removed in D122099


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122002