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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_all_of.h:35
+  _LIBCPP_HIDE_FROM_ABI constexpr static
+  bool __go(_Iter __first, _Sent __last, _Pred& __pred, _Proj& __proj) {
+    for (; __first != __last; ++__first) {
----------------
ldionne wrote:
> I suggested naming these helpers `__all_of_impl` in a previous review (I think we had agreed on the naming scheme). I assume this slipped because you already had this patch locally when I made the comment, but please make sure to rebase all your local patches with my comments to make sure nothing slips through the cracks.
> 
> You can then ping me when you think this is ready for review.
I thought the new naming scheme was only for the ones that are used in multiple CPOs. I'll update my current patches!


================
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{}); };
+
----------------
var-const wrote:
> Why is `std::identity` passed explicitly here, instead of relying on the default argument?
There is no default argument. This is the predicate.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp:65
+constexpr void test_iterators() {
+  { // simple test
+    {
----------------
var-const wrote:
> 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.
I knew I missed a few cases. These tests felt way to short.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123016