[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 __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);
----------------
var-const wrote:
> 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)?
Both should return the leftmost element. I'm not sure how you got to the rightmost element by swapping the arguments. Did you think of less-or-equal maybe?


================
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);
----------------
var-const wrote:
> 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.
> 
I think the `&&` part is only interesting for returning `ranges::dangling`. A comparator that only takes the arguments by lvalue isn't allowed. See `std::indirect_strict_weak_order` for details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122002