Details
- Reviewers
ldionne var-const Mordante - Group Reviewers
Restricted Project - Commits
- rG0e3dc1a52ffe: [libc++] Implement ranges::{all, any, none}_of
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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. |
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)?
| |
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. |
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. |
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)? |
- 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. |
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. |
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.
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.