Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Iterator::partition_in_place() and is_partitioned() #62278

Merged
merged 7 commits into from Jul 10, 2019

Conversation

@cuviper
Copy link
Member

commented Jul 1, 2019

partition_in_place() swaps &mut T items in-place to satisfy the
predicate, so all true items precede all false items. This requires
a DoubleEndedIterator so we can search from front and back for items
that need swapping.

is_partitioned() checks whether the predicate is already satisfied.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

inplace_partition personally sounds like a better name to me than partition_mut.

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

These are inspired by C++ partition and is_partitioned, spurred by this question about mimicking #include <algorithm>.

This requires a DoubleEndedIterator so we can search from front and back for items that need swapping.

Implementation note: C++ allows partition with only ForwardIt, but they have to use two mutable iterators (cursors) to implement it, which Rust can't really allow. The best I could imagine is keeping a queue of &mut T we've already seen, eligible for swapping, which would require allocation (non-core) and seems generally distasteful.

C++ does talk about bi-directional modes too, which I imagine would work like what I have here.

@Mark-Simulacrum

inplace_partition personally sounds like a better name to me than partition_mut.

I'm open to renaming. I don't think an inplace_ prefix is used anywhere else, but there are examples of ptr::drop_in_place and Alloc::grow_in_place and shrink_in_place.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Ah -- in that case I think partition_in_place is probably the better name. I thought we had something similar :) I think the _mut suffix is a bit odd, since the iterator must be &mut to even be able to call the method.

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

I would have used partition alone if it didn't exist! 🙂 Ours is similar to C++'s partition_copy.

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

There's also itertools::partition() -- at a (too-brief) glance I had assumed it was just an IntoIterator wrapper on Iterator::partition, like many of their other free functions, but it actually does the same as my partition_mut, in-place. They return the usize index of the partition split, which seems useful.

I wonder if we might also want an index from is_partitioned, or another method along these lines like find_partition(pred) -> Option<usize>. C++ has partition_point returning an iterator, but we can approximate that already with skip_while(predicate).

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

_in_place does seem like the better naming.

I would have used partition alone if it didn't exist! 🙂 Ours is similar to C++'s partition_copy.

(That wouldn't have been very appropriate imo as our naming is primarily Haskell based...)

@cuviper cuviper changed the title Add Iterator::partition_mut() and is_partitioned() Add Iterator::partition_in_place() and is_partitioned() Jul 1, 2019

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

OK, that's 3 votes for partition_in_place -- changed!

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This looks great to me, thanks @cuviper! Could a few more tests be included as well beyond the doc tests?

Otherwise API-wise would it be feasible to return a usize from partition_in_place to indicate the size of one of the partitions? That way for array indexing you could easily split at the index aftewards in theory

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

Thanks for the review -- sorry I didn't get back to this yet. I can definitely add more tests, and I think partition_in_place -> usize should be feasible too.

cuviper and others added some commits Jul 1, 2019

Add Iterator::partition_mut() and is_partitioned()
`partition_mut()` swaps `&mut T` items in-place to satisfy the
predicate, so all `true` items precede all `false` items. This requires
a `DoubleEndedIterator` so we can search from front and back for items
that need swapping.

`is_partitioned()` checks whether the predicate is already satisfied.
Capitalize example comment
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

@cuviper cuviper force-pushed the cuviper:iter-partition branch from 49da3c2 to 265e3a6 Jul 9, 2019

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

OK, ready for another review.

// FIXME: should we worry about the count overflowing? The only way to have more than
// `usize::MAX` mutable references is with ZSTs, which aren't useful to partition...

// These closure "factory" functions exist to avoid genericity in `Self`.

This comment has been minimized.

Copy link
@cuviper

cuviper Jul 9, 2019

Author Member

On this, see also #62429.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Looks great to me, thanks @cuviper! r=me with the tracking issue numbers filled in

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Done!

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

📌 Commit 7171c83 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019

Rollup merge of rust-lang#62278 - cuviper:iter-partition, r=alexcrichton
Add Iterator::partition_in_place() and is_partitioned()

`partition_in_place()` swaps `&mut T` items in-place to satisfy the
predicate, so all `true` items precede all `false` items. This requires
a `DoubleEndedIterator` so we can search from front and back for items
that need swapping.

`is_partitioned()` checks whether the predicate is already satisfied.

bors added a commit that referenced this pull request Jul 10, 2019

Auto merge of #62555 - Centril:rollup-ti46adx, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #61853 (Emit warning when trying to use PGO in conjunction with unwinding on …)
 - #62278 (Add Iterator::partition_in_place() and is_partitioned())
 - #62283 (Target::arch can take more than listed options)
 - #62393 (Fix pretty-printing of `$crate` (take 4))
 - #62474 (Prepare for LLVM 9 update)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

⌛️ Testing commit 7171c83 with merge 0324a2b...

@bors bors merged commit 7171c83 into rust-lang:master Jul 10, 2019

3 of 4 checks passed

homu Testing commit 7171c83ab234926d620c4869eeac90b393833ef2 with merge 0324a2b309cd66cb7bd4a156bd0b84cb136e254f...
Details
pr Build #20190709.65 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.