[libcxx-commits] [PATCH] D123016: [libc++] Implement ranges::{all, any, none}_of

[Konstantin Varlamov via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp:30
+template <class It, class Sent = sentinel_wrapper<It>>
+concept HasAllOfIt = requires(It first, Sent last) { std::ranges::all_of(first, last, std::identity{}); };
+
----------------
Why is `std::identity` passed explicitly here, instead of relying on the default argument?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp:65
+constexpr void test_iterators() {
+  { // simple test
+    {
----------------
If I'm reading this correctly, the current tests would pass even if the implementation unconditionally returned `true`. Can you please add a few tests with a predicate that depends on the element (e.g. returns whether the number is even)?
- no element satisfies the condition;
- all elements satisfy the condition;
- only some elements satisfy the condition.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp:68
+      int a[] = {1, 2, 3, 4};
+      std::same_as<bool> auto ret = std::ranges::all_of(It(a), Sent(It(a + 4)), [](int) { return true; });
+      assert(ret);
----------------
Nit: I still feel we should use `decltype(auto)` uniformly with `same_as`. Yes, it's exceedingly unlikely that this algorithm could return a reference, but it would be a simple rule to follow, always correct and requiring no decision-making.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp:30
+template <class It, class Sent = sentinel_wrapper<It>>
+concept HasAllOfIt = requires(It first, Sent last) { std::ranges::any_of(first, last, std::identity{}); };
+
----------------
Nit: `s/AllOf/AnyOf/` (throughout the file).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp:30
+template <class It, class Sent = sentinel_wrapper<It>>
+concept HasAllOfIt = requires(It first, Sent last) { std::ranges::none_of(first, last, std::identity{}); };
+
----------------
Same nit re. the copy-paste error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123016