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

fix(store::TreeMap): remove of the entry API now correctly updates the tree root when changed #995

Merged
merged 5 commits into from May 26, 2023

Conversation

austinabell
Copy link
Contributor

The bug was within the remove method of the entry API, which removed the element but did not update the root based on what was returned from do_remove.

This was a weird side effect to have to uphold after calling do_remove, so this seems like the correct refactor rather than just updating the root within that function.

Closes #993

@austinabell
Copy link
Contributor Author

Opinionated if this change is also backported to the collections::TreeMap.

self.root = self.do_remove(key);

There is no bug in that collection because the root is updated whenever that function is called, but an argument could be made to make this change for the off-chance changes are made to it that don't uphold this assumption.

@frol
Copy link
Collaborator

frol commented May 24, 2023

@itegulov @ChaoticTempest Kindling pinging you as code owners to review this PR.

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

LGTM

@frol frol merged commit 2db863c into master May 26, 2023
15 checks passed
@frol frol deleted the austin/fix_treemap_entry branch May 26, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TreeMap panic on inser after remove last value [unstable]
3 participants