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 bug in remove_index for OrdMap #141

Merged
merged 3 commits into from Jul 14, 2020

Conversation

dsarlis
Copy link
Contributor

@dsarlis dsarlis commented Jul 1, 2020

Fixes #124

When removing a value and rebalancing the tree, a value could be moved to a node that doesn't have the required free capacity, therefore resulting in a panic.

@dsarlis
Copy link
Contributor Author

dsarlis commented Jul 1, 2020

This is fixing the repro mentioned in the issue and there's only a single test failing for im-rc when I ran the tests locally. I believe this patch is in the right direction, so any help to make sure all tests pass is welcome.

@bodil bodil self-assigned this Jul 14, 2020
@bodil
Copy link
Owner

bodil commented Jul 14, 2020

I've got no tests failing after this fix, including after I added the repro case for #124, so I'll take that as a positive sign. 😊

@bodil bodil merged commit c35a913 into bodil:master Jul 14, 2020
@bodil bodil mentioned this pull request Jul 14, 2020
@dsarlis
Copy link
Contributor Author

dsarlis commented Jul 15, 2020

Thanks @bodil! Yes, I'd noticed that the tests hadn't failed on CI either, but I remember there was a failure locally for me. Maybe I did something wrong though. If all tests pass this sounds like a good sign to me too.

@bodil
Copy link
Owner

bodil commented Jul 15, 2020

CI is finding new bugs now, though, but I can't say whether they were caused by this commit or would have been there with the old code too - most likely the latter. They seem extremely nondeterministic, in that I'm not able to reproduce them on my local machine from the dataset the CI proptest is reporting - as the tests are using a std::collections::HashMap as a control, it might very well be down to the exact ordering of inputs as determined by the randomised seed the hasher is using... oh joy. I've redone the tests, and hopefully the fuzzer finds a reproducible case soon.

Anyway, this definitely fixed one of the bugs, so good work. 😊

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.

Panic when using OrdMap
2 participants