BinaryHeap: implement IterMut

[donkeyteethUX]

I recently had a need for this and decided to go ahead and implement it.

Some notes:

• iter_mut() is implemented for BinaryHeap<T: Ord>. Generalizing to BinaryHeap<T> would require a specialized drop. This limitation might be fine (not sure why you would use a heap without ordering anyway) but it feels a little odd.
• I used a type from the btree module (DormantMutRef). It might make sense to move that file.
• In principle, IterMut can be made double-ended. However it would greatly complicate things, and since the iteration order is mostly arbitrary, I don't see much point.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

[m-ou-se]

This seems like quite a specialized method and type, and not at all like the other .iter() and .iter_mut() methods, because of its rebuild-on-drop behaviour.

To me, it doesn't seem like the kind of common operation that warrants its own method and type, since the same can be achieved with .into_vec() and BinaryTree::from to mutate the elements through Vec's interface.

[donkeyteethUX]

@m-ou-se Thanks for taking a look. It's true you can modify through Vec, but you have to write

let mut heap_vec = heap.into_vec();
heap_vec.iter_mut().for_each(update_entry);
heap = BinaryHeap::from(heap_vec);

in lieu of heap.iter_mut().for_each(update_entry).

This seems sub-optimal because

• The API doesn't make it obvious that this is currently the best way to mutably iterate. It isn't clear without digging that this doesn't add much overhead (at least when full a rebuild is required).
• It is less efficient than IterMut in the case where only some elements were modified, because the entire heap is rebuilt. i.e. IterMut would make something like heap.iter_mut().take(10).for_each(update_entry) to be O(log(n)) instead of O(n).

As far as the drop behavior, I think it's fairly unsurprising, especially since we already have PeekMut which behaves similarly.

[m-ou-se]

I don't think that pattern is common enough to warrant a new method and type just to be able to write those three lines as one.

As far as the drop behavior, I think it's fairly unsurprising, especially since we already have PeekMut which behaves similarly.

It's surpsing behaviour for something we call iter_mut() / IterMut. Iterators normally don't do anything like this.

It is less efficient than IterMut in the case where only some elements were modified, because the entire heap is rebuilt. i.e. IterMut would make something like heap.iter_mut().take(10).for_each(update_entry) to be O(log(n)) instead of O(n).

How common is that? In which use cases does that happen, and what is the alternative?

If you want to add a new API to the standard library, you'll have to show that the problem it solves is big enough to be worth it. If something is uncommon and there is already a safe and reasonable alternative, it usually does not meet the bar for addition to the standard library.

[donkeyteethUX]

Yeah I think you're right. Thanks again for taking the time (also for all the hard work on std::sync).

[m-ou-se]

Yeah I think you're right.

In that case, I'm closing this PR. (But please don't let that discourage you from future contributions!)

(Note that next time you want to propose a new API for the standard library, feel free open an issue on the libs-team repository to get feedback on the public interface before implementing it, to potentially save yourself some work.)