Stabilize feature(binary_heap_into_iter_sorted) #76234
Conversation
|
should we stabilize drain_sorted in the same step? the two methods feel pretty similar. |
|
@matklad There were concerns about that version in the tracking issue and the use I ran into didn't need it, so I didn't include it here. We certainly could, if people wanted, though. |
|
Revisiting this after the original PR, the different ordering of One way to resolve this could be to consider deprecating Another way could be to tweak this API a bit would be to slot a type in-between the iterators that lives in pub struct Sorted<T>(BinaryHeap<T>);
pub struct SortedMut<'a, T>(&'a mut BinaryHeap<T>);
// current into_iter_sorted
impl<T> IntoIterator for Sorted<T> {
type Item = T;
}
// equivalent to SortedMut<'a, T> as IntoIterator
impl<'a, T> IntoIterator for &'a mut Sorted<T> {
type Item = T;
}
impl<'a, T> IntoIterator for SortedMut<'a, T> {
type Item = T;
}
impl<'a, T> SortedMut<'a, BinaryHeap<T>> {
// current drain_sorted
pub fn drain(self) -> SortedMutDrain<'a, T>;
}
impl<T> BinaryHeap<T> {
pub fn into_sorted(self) -> Sorted<T>;
pub fn sorted_mut(&mut self) -> SortedMut<T>;
}It would then be called like: // instead of into_iter_sorted
for item in heap.into_sorted() {
..
}
// into_sorted_vec stays around
let v = heap.into_sorted_vec();
// instead of drain_sorted
for item in heap.sorted_mut().drain() {
..
}
// like drain_sorted, but doesn't clear the heap
let max_5 = heap.sorted_mut().into_iter().take(5).collect::<Vec<_>>();That's extra machinery and indirection though we may not think is worthwhile. I do think it's worth exploring though, since |
|
Hmm, Your point about |
Do you think the way we'd want to do this would be to introduce a new which then covers: What do you think?
I was curious about this so thought I'd give it a try. A simple microbenchmark seems to support the idea that With a heap size of 10 elements: With a heap size of 100 elements: With a heap size of 1000 elements: #![feature(test, binary_heap_into_iter_sorted)]
extern crate test;
use std::collections::BinaryHeap;
// tweak this to change the size of the heap
const N: usize = 1000;
fn interleaved() -> BinaryHeap<usize> {
let mut heap = BinaryHeap::new();
for (a, b) in (0..N / 2).zip((N / 2..N).into_iter().rev()) {
heap.push(a);
heap.push(b);
}
heap
}
fn already_sorted() -> BinaryHeap<usize> {
let mut heap = BinaryHeap::new();
for i in (0..N).into_iter().rev() {
heap.push(i);
}
heap
}
#[bench]
fn interleaved_heap_into_sorted_vec(b: &mut test::Bencher) {
let heap = interleaved();
b.iter(|| {
let heap = heap.clone();
heap.into_sorted_vec()
});
}
#[bench]
fn interleaved_heap_collect_sorted_vec(b: &mut test::Bencher) {
let heap = interleaved();
let mut sorted = Vec::new();
sorted.extend(heap.clone().into_iter_sorted());
b.iter(|| {
let heap = heap.clone();
sorted.clear();
// Unlike the `into_vec` methods, this creates a new `Vec` each time
// We amortize the cost a little be re-using it
sorted.extend(heap.into_iter_sorted());
test::black_box(&sorted);
});
}
#[bench]
fn interleaved_heap_into_vec_sort_unstable(b: &mut test::Bencher) {
let heap = interleaved();
b.iter(|| {
let mut heap = heap.clone().into_vec();
heap.sort_unstable();
heap
});
}
#[bench]
fn already_sorted_heap_into_sorted_vec(b: &mut test::Bencher) {
let heap = already_sorted();
b.iter(|| {
let heap = heap.clone();
heap.into_sorted_vec()
});
}
#[bench]
fn already_sorted_heap_collect_sorted_vec(b: &mut test::Bencher) {
let heap = already_sorted();
let mut sorted = Vec::new();
sorted.extend(heap.clone().into_iter_sorted());
b.iter(|| {
let heap = heap.clone();
sorted.clear();
// Unlike the `into_vec` methods, this creates a new `Vec` each time
// We amortize the cost a little be re-using it
sorted.extend(heap.into_iter_sorted());
test::black_box(&sorted);
});
}
#[bench]
fn already_sorted_heap_into_vec_sort_unstable(b: &mut test::Bencher) {
let heap = already_sorted();
b.iter(|| {
let mut heap = heap.clone().into_vec();
heap.sort_unstable();
heap
});
}Should we update the implementation of |
Oh wow, that means that max heap really doesn’t make sense.
As well as just is the fact that iter_sorted returns reversely-sorted Items. I would totally expect anything _sorted to be ascending. Should we stick a |
|
☔ The latest upstream changes (presumably #70793) made this pull request unmergeable. Please resolve the merge conflicts. |
That tripped me up too. Since it's role is to call |
|
For record, the reason I picked the method name Discoverability in IDE use case.
|
In rust iterator design, My opinion is that the method is a variant of |
It's an interesting API redesign, but
pub struct Unsorted<T>(BinaryHeap<T>);
pub struct UnsortedMut<'a, T>(&'a mut BinaryHeap<T>);In this new world, we'd use iterator like below (LHS).
But actualy I won't implement
|
For the order of name, I feel like For example, min-max-heap crate provide the following set of methods [1]:
They are highly discoverable. I'm not endorsing |
I actually think of it slightly differently. I can appreciate the discoverability use-case preferring suffixes, and do think Since I think |
|
I agree that
|
|
I'm going to withdraw this PR, since the conversation has convinced me that things aren't ready yet here. My personal conclusions:
|
This came up on an itertools issue, and looks like it's baked sufficiently, so I figured I'd ask about stabilization.
This would stabilize the following API:
https://doc.rust-lang.org/nightly/std/collections/struct.BinaryHeap.html#method.into_iter_sorted
There were comments about wishing that the ordinary
.into_iter()could just be the sorted one, but.iter()cannot be in that order so that's not necessarily good. And anyways,IntoIteralready implementsDoubleEndedIteratorso we can't really change the order even if we wanted to.So I think the remaining potential questions are naming (it was originally proposed as
into_iter_orderedbut went in asinto_iter_sorted, which I agree is more consistent with elsewhere in the library) and the standard whether this is useful enough (one could certainly usefrom_fnon the heap's pop, but that's potentially suboptimal as it doesn't have asize_hint).This needs an FCP, so picking a libs member explicitly:
r? @KodrAus
Tracking issue #59278 (which also covers a drain version which I've intentionally not included here)