[libc++][ranges] implement std::ranges::includes
Details
- Reviewers
philnik var-const jdoerfert - Group Reviewers
Restricted Project - Commits
- rGc559964d85e8: [libc++][ranges] implement `std::ranges::includes`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Almost LGTM -- the only important comment is about sentinel types in tests.
libcxx/docs/Status/RangesAlgorithms.csv | ||
---|---|---|
32 | Nit: please include the patch link. | |
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp | ||
49 | Nit: include <utility>. | |
109 | I'm a bit concerned that we never check the pretty common case where In1 and Sent1 are the same type. Consider making Sent1 and Sent2 template parameters. If the test takes too long, I'd rather omit some less interesting iterator types (for partition_copy, I only use iterator types with the weakest and the strongest constraints, with the idea that if both cpp20_input_iterator and random_access_iterator work, then it's very unlikely that forward_iterator or bidirectional_iterator won't work). | |
400 | Optional: there's some prior art to reduce boilerplate here in test_support/counting_predicates.h. I used it in https://reviews.llvm.org/D130070 (with some additions) and it seemed to reduce verbosity a little. This can easily be done in a follow-up. |
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp | ||
---|---|---|
239–240 | Nit: we can remove these TODOs since you have tests below with a custom comparator and custom projection, right? |
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp | ||
---|---|---|
239–240 | +1 -- sorry I somehow missed that. |
Thanks for addressing the feedback!
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp | ||
---|---|---|
240 | This is pretty neat. You could also do something like if constexpr (std::sentinel_for<Iter2, Iter2>) to decide whether the iterator can be used as its own sentinel type (just throwing it out there, this LGTM). |
Nit: please include the patch link.