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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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>);
+
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589