Add BinaryHeap::retain

[Conaclos]

Implement #666

This PR adds the BinaryHeap::retain method.
I didn't base my work on #345 because the implementation seems wrong to me.

I added an internal method BinaryHeap::remove that is now used by both BinaryHeap::retain and BinaryHeap::pop_unchecked.
In the process I improved its implementation.

One alternative implementation could be to traverse the internal vector in reverse order. This could reduce the number of shifted elements if retain removes multiple elements. However, this would make the implementation much more complex.

[zeenix]

I didn't base my work on #345 because the implementation seems wrong to me.

How so?

[Conaclos]

How so?

The implementation doesn't preserve the order invariant of the binary heap.
For example, if you take the binary max-heap [8, 2, 5] and ask to retain only the numbers that are different of 8, then you end up with [2, 5], which is incorrect.

[zeenix]

For example, if you take the binary max-heap [8, 2, 5] and ask to retain only the numbers that are different of 8, then you end up with [2, 5], which is incorrect.

Huh? Is it? 🤔 https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=98dfdde25b2a102d7146761f6d5294db

[Conaclos]

I will add some tests for sift_down_to_bottom because the function seems untested with arguments (pos) greater than 0.

[Conaclos]

Huh? Is it? 🤔 https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=98dfdde25b2a102d7146761f6d5294db

Your example is wrong because you called into_sorted_vec() in the assertion. It will always return [2, 5].
Instead, try: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=dccba979ca3526f18a2a2dea5f93130f

[zeenix]

Your example is wrong because you called into_sorted_vec() in the assertion. It will always return [2, 5].
Instead, try: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=dccba979ca3526f18a2a2dea5f93130f

Ah right. That was from the std doc example I started off from.

[Conaclos]

Ready for review!

[zeenix]

Only tiny nits and then it's good from my side.

[Conaclos]

@zeenix Is there any concerns preventing merging this feature?

[zeenix]

@zeenix Is there any concerns preventing merging this feature?

Not from my side but I was hoping for another review from someone else.

[sgued]

LGTM besides two small nits.

Thank you!

[sgued]

Please make this a // SAFETY: comment, that way we can progressively work towards enabling #![deny(clippy::undocumented_unsafe_blocks)]

[sgued]

Suggested change

	// This test depends on implementation details.
	// This test depends on private APIs.

I spent some times figuring out which implementation detail regarding order or something was impactful, I think "private APIs" is clearer and is already used in another comment.