Page MenuHomePhabricator

[libc++][ranges] implement `std::ranges::includes`

Authored by huixie90 on Jul 19 2022, 1:02 PM.



[libc++][ranges] implement std::ranges::includes

Diff Detail

Event Timeline

huixie90 created this revision.Jul 19 2022, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 1:02 PM
huixie90 requested review of this revision.Jul 19 2022, 1:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const requested changes to this revision.Jul 19 2022, 8:09 PM

Almost LGTM -- the only important comment is about sentinel types in tests.


Nit: please include the patch link.


Nit: include <utility>.


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


Optional: there's some prior art to reduce boilerplate here in test_support/counting_predicates.h. I used it in (with some additions) and it seemed to reduce verbosity a little. This can easily be done in a follow-up.

This revision now requires changes to proceed.Jul 19 2022, 8:09 PM
jloser added a subscriber: jloser.Jul 19 2022, 8:35 PM
jloser added inline comments.

Nit: we can remove these TODOs since you have tests below with a custom comparator and custom projection, right?

var-const added inline comments.Jul 19 2022, 8:36 PM

+1 -- sorry I somehow missed that.

huixie90 updated this revision to Diff 446275.Jul 20 2022, 3:05 PM
huixie90 marked 4 inline comments as done.

address review issues

var-const accepted this revision.Jul 20 2022, 11:12 PM

Thanks for addressing the feedback!


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

This revision is now accepted and ready to land.Jul 20 2022, 11:12 PM
This revision was automatically updated to reflect the committed changes.