This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

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.

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

var-const added inline comments.Jul 19 2022, 8:36 PM
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/includes/ranges_includes.pass.cpp
239–240

+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!

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

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.