[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:43-44
+static_assert(!HasMin<int>);
+static_assert(!HasMin<NoLessThanOp>);
+static_assert(!HasMin<NotTotallyOrdered>);
+
----------------
philnik wrote:
> Quuxplusone wrote:
> > You probably intended
> > ```
> > static_assert( HasMin<int(&)[10]>);
> > static_assert(!HasMin<NoLessThanOp(&)[10]>);
> > static_assert(!HasMin<NotTotallyOrdered(&)[10]>);
> > ```
> > and/or
> > ```
> > static_assert(!HasMin<std::initializer_list<NoLessThanOp>>);
> > static_assert(!HasMin<std::initializer_list<NotTotallyOrdered>>);
> > ```
> > FYI, I'm getting more and more uncomfortable with this `HasXxx` pattern, because it makes it impossible to test rvalue types. I think you should switch, at least in new code, to
> > ```
> > static_assert(std::invocable<decltype(std::ranges::min), std::initializer_list<int>>);
> > static_assert(std::invocable<decltype(std::ranges::min), int(&)[10]>);
> > static_assert(std::invocable<decltype(std::ranges::min), int(&&)[10]>); // impossible to express right now
> > ```
> I copy/pasted that part, so it's probably also not tested properly in the other test.
> 
> > I'm getting more and more uncomfortable with this HasXxx pattern
> `concept HasMin = requires(T&& t) { std::ranges::min(std::forward<T>(t)); }` should work for these cases, right?
Yes, it should; or even `concept HasMin = requires { std::ranges::min(std::declval<T>()); }`. I'm ambivalent as to the readability of that versus a straight `std::invocable<RangeMinT, ...>`, so, either is fine AFAIC.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:75
+    };
+    [[maybe_unused]] std::same_as<int> auto ret = std::ranges::min({1, 2, 3}, comparator, projection);
+    assert(compares == 2);
----------------
philnik wrote:
> Quuxplusone wrote:
> > This should be `std::same_as<int> decltype(auto)` if that's allowed (I think it probably isn't), or else write it out longhand with `decltype(auto) ret = ~~~ ASSERT_SAME_TYPE(decltype(ret), ~~~)`. What we're worried about here is that this overload of `ranges::min` should return exactly `int`, not a `const int&` that might dangle.
> At least clang is happy with `concept decltype(auto)`. It also works as expected.
> At least clang is happy with `concept decltype(auto)`

Yep, Godbolt says everyone's happy with it, so, OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589