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

Mon Mar 21 23:37:53 PDT 2022

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:19
 Search,min,Nikolas Klauser,`D119589 <https://llvm.org/D119589>`_,✅
-Search,max,Not assigned,n/a,Not started
+Search,max,Nikolas Klauser,n/a,✅
 Search,minmax,Not assigned,n/a,Not started
----------------
Nit: can you add a link to this patch as well?

================
Comment at: libcxx/include/__algorithm/ranges_max.h:50
+  _Tp operator()(initializer_list<_Tp> __il, _Comp __comp = {}, _Proj __proj = {}) const {
+    _LIBCPP_ASSERT(__il.begin() != __il.end(), "initializer_list must contain at least one element");
+    auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) { return std::invoke(__comp, __rhs, __lhs); };
----------------
Very optional nit: personally, I usually prefer to separate any initial error handling from the main body of the function by a blank line (so in this case, I would add a blank line after the assertion). However, it's really a personal preference thing, so feel free to ignore if you don't feel it makes a difference.

================
Comment at: libcxx/include/__algorithm/ranges_max.h:51
+    _LIBCPP_ASSERT(__il.begin() != __il.end(), "initializer_list must contain at least one element");
+    auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) { return std::invoke(__comp, __rhs, __lhs); };
+    return *ranges::__min_element_impl(__il.begin(), __il.end(), __comp_lhs_rhs_swapped, __proj);
----------------
I think there's an important nuance in how equivalent elements are handled. IIUC, `min_element` returns the rightmost of equivalent elements, whereas `max` needs to return the leftmost -- therefore, with the arguments swapped, we will get the correct behavior for `max`. Can you please add a comment to that effect (assuming my reasoning is correct)?

================
Comment at: libcxx/include/__algorithm/ranges_max.h:67
+      auto __comp_lhs_rhs_swapped = [&](auto&& __lhs, auto&& __rhs) { return std::invoke(__comp, __rhs, __lhs); };
+      return *ranges::__min_element_impl(__first, __last, __comp_lhs_rhs_swapped, __proj);
+    } else {
----------------
Nit: move `__first` and `__last`?

================
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:
> 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.
Do we have tests to make sure a comparator that takes the argument by lvalue works? (if not, please add them)

I think ranges often use forwarding references simply to avoid providing several overloads for different types of references (sort of similar in spirit to how the "take-by-value-and-move" idiom allows not writing two overloads for rvalue and const lvalue). This //seems// to apply here, but @ldionne would know better.

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max.pass.cpp:215

-  { // check that the first smallest element is returned
-    { // where the first element is the smallest
+  { // check that the first largest element is returned
+    { // where the first element is the largest
----------------
Hmm, I don't see the equivalent test for the `initializer_list` overload, but perhaps I'm just not seeing it?

================
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:
> 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.
> 
> 
+1 for using `int i[1]`, which is clearly valid and adds very little boilerplate.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122002