This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement ranges::{all, any, none}_of
ClosedPublic

Authored by philnik on Apr 4 2022, 2:27 AM.

Details

Diff Detail

Event Timeline

philnik created this revision.Apr 4 2022, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 2:27 AM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Apr 4 2022, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 2:27 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Apr 4 2022, 10:01 AM
libcxx/include/__algorithm/ranges_all_of.h
36

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.

var-const requested changes to this revision.Apr 4 2022, 1:06 PM
var-const added inline comments.
libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
31

Why is std::identity passed explicitly here, instead of relying on the default argument?

66

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.
69

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.

libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp
31

Nit: s/AllOf/AnyOf/ (throughout the file).

libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp
31

Same nit re. the copy-paste error.

This revision now requires changes to proceed.Apr 4 2022, 1:06 PM
philnik marked an inline comment as done.Apr 4 2022, 1:15 PM
philnik added inline comments.
libcxx/include/__algorithm/ranges_all_of.h
36

I thought the new naming scheme was only for the ones that are used in multiple CPOs. I'll update my current patches!

libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
31

There is no default argument. This is the predicate.

66

I knew I missed a few cases. These tests felt way to short.

var-const added inline comments.Apr 4 2022, 3:24 PM
libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
31

Ah, right -- seeing std::identity, I mistook it for the projection. Perhaps change it to something else that looks more like a predicate (you could create a simple global functor like AlwaysTrue or similar and reuse it across the tests if you'd like)?

philnik updated this revision to Diff 420519.Apr 5 2022, 8:30 AM
philnik marked 6 inline comments as done.
  • Address comments
libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
31

I'd much rather just use a simple lambda. I can add a TODO so we change it once AppleClang supports them in this context if you want.

var-const accepted this revision as: var-const.Apr 5 2022, 2:19 PM

LGTM once the CI is green, but I'd wait for one other review before merging.

libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
31

FWIW, I still feel that using std::identity as a predicate is unnecessarily confusing, but this is non-blocking.

philnik updated this revision to Diff 426295.May 1 2022, 5:21 AM
philnik marked an inline comment as done.
  • Try to fix CI
philnik updated this revision to Diff 431145.May 21 2022, 9:36 AM
philnik marked an inline comment as done.
  • Rebased
  • Fix CI
ldionne accepted this revision.May 26 2022, 7:34 AM

I don't think we have any tests that these algorithms are actually function objects that can be e.g. passed as a function argument. Didn't we have a test file that checked all algorithms and views for being niebloids? If so, let's update it. If not, let's create one.

This revision is now accepted and ready to land.May 26 2022, 7:34 AM
This revision was automatically updated to reflect the committed changes.