Skip to content

Add BinaryHeap::retain - #668

Open
Conaclos wants to merge 6 commits into
rust-embedded:mainfrom
Conaclos:binaryheap_retain
Open

Add BinaryHeap::retain#668
Conaclos wants to merge 6 commits into
rust-embedded:mainfrom
Conaclos:binaryheap_retain

Conversation

@Conaclos

@Conaclos Conaclos commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

@Conaclos Conaclos changed the title Add BinaryHeap::retain Add BinaryHeap::retain Jul 8, 2026
@Conaclos
Conaclos force-pushed the binaryheap_retain branch 4 times, most recently from 87e788b to 64e77df Compare July 8, 2026 15:17
@zeenix

zeenix commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

How so?

@Conaclos

Conaclos commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

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

zeenix commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

Conaclos commented Jul 8, 2026

Copy link
Copy Markdown
Author

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

@Conaclos

Conaclos commented Jul 8, 2026

Copy link
Copy Markdown
Author

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

zeenix commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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.

@zeenix zeenix mentioned this pull request Jul 8, 2026
Closed
@Conaclos
Conaclos force-pushed the binaryheap_retain branch 3 times, most recently from 4c41991 to d8e3a5c Compare July 21, 2026 20:02
@Conaclos
Conaclos force-pushed the binaryheap_retain branch from 985fee8 to beb1189 Compare July 21, 2026 20:10
@Conaclos
Conaclos force-pushed the binaryheap_retain branch 2 times, most recently from 58678fe to 1535316 Compare July 21, 2026 20:12
@Conaclos

Conaclos commented Jul 21, 2026

Copy link
Copy Markdown
Author

Ready for review!

zeenix
zeenix requested changes Jul 21, 2026
View reviewed changes

@zeenix zeenix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread CHANGELOG.md Outdated
Comment thread src/binary_heap.rs
Comment thread src/binary_heap.rs
zeenix
zeenix previously approved these changes Jul 22, 2026
View reviewed changes
@Conaclos

Conaclos commented Jul 27, 2026

Copy link
Copy Markdown
Author

@zeenix Is there any concerns preventing merging this feature?

@zeenix

zeenix commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

@zeenix Is there any concerns preventing merging this feature?

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

@zeenix
zeenix requested a review from a team July 27, 2026 11:59
sgued
sgued reviewed Jul 28, 2026
View reviewed changes

@sgued sgued left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides two small nits.

Thank you!

Comment thread src/binary_heap.rs Outdated
Comment thread src/binary_heap.rs Outdated
@mmilata mmilata mentioned this pull request Jul 29, 2026
Open
5 tasks
@Conaclos

Conaclos commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

@sgued Fixed.

I took the opportunity of making unsafe the relevant functions, adding missing unsafe blocks and SAFETY comments. This improves compatibility with clippy::undocumented_unsafe_blocks and clippy::multiple_unsafe_ops_per_block.

I added PeekMutInner::new that allows checking the type invariant.
Ideally, this type should be moved to a dedicated module to protect its instantiation.
I renamed PeekMutInner::sift into PeekMutInner::removed (inverting the boo logic) to make clearer its purpose.

Last, I turned some conditionals into early returns to make them cold paths.

@Conaclos
Conaclos force-pushed the binaryheap_retain branch from 9fb7ae8 to 7de39e5 Compare August 1, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgued sgued sgued requested changes
@zeenix zeenix zeenix left review comments

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Conaclos @zeenix @sgued