This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement `ranges::{,stable_}partition`.
ClosedPublic

Authored by var-const on Jul 13 2022, 2:13 AM.

Diff Detail

Event Timeline

var-const created this revision.Jul 13 2022, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 2:13 AM
var-const requested review of this revision.Jul 13 2022, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 2:13 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 444191.Jul 13 2022, 2:14 AM

Forgotten file

var-const updated this revision to Diff 444192.Jul 13 2022, 2:15 AM

Patch link.

var-const updated this revision to Diff 444193.Jul 13 2022, 2:18 AM

Uncomment omitted test.

var-const updated this revision to Diff 444194.Jul 13 2022, 2:19 AM

Fix the CI.

var-const updated this revision to Diff 445385.Jul 17 2022, 9:34 PM

Rebase and fix the CI.

Rebase on main and make the new algorithms support proxy iterators.

Version forgotten file.

Fix the CI.

huixie90 accepted this revision.Jul 18 2022, 2:19 AM

The algorithm LGTM. Regarding testing, I still prefer to have extra tests to verify iter_move and/or iter_swap is correctly used

libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_stable_partition.pass.cpp
236

as these algorithms involves iter_move, I suggests to add tests to verify the outputs before we can figure out an automated way to catch in via a compile-time tests. so here , we could have

std::array<MoveOnly, N> a{...];
ProxyRange r{a};
std::ranges::partition(r);
// verify that the original `a` is partitioned as the expected result

(actually even without verifying the result, the compiler would catch std::move(*it) because std::move(*it) would copy the MoveOnly for Proxy<MoveOnly&>&& and MoveOnly is not copyable

var-const added inline comments.Jul 18 2022, 8:26 PM
libcxx/include/__algorithm/iterator_operations.h
92 ↗(On Diff #445388)
libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_stable_partition.pass.cpp
236

I'd prefer to do these checks en masse: https://reviews.llvm.org/D130057

@philnik I'm going to merge this, but happy to address any comments you might have in a follow-up.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 18 2022, 9:06 PM
This revision was automatically updated to reflect the committed changes.