Add ordered_remove with O(n) perf.
#15
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This does look good to me, just not sure if we need it. I think it is ultimately needed, the OrderMap has a lot of Vec like functionality going already. |
Merged
|
Since some time has passed, you can disregard review comments here. But I'll put down my thoughts so that I don't forget them, for when I come back to this. |
bluss
reviewed
Dec 31, 2017
| self.index = updater(self.index as usize) as u64; | ||
| } else { | ||
| let (index, hash) = split_lo_hi(self.index); | ||
| self.index = updater(index as usize) as u64 | ((hash as u64) << 32); |
There was a problem hiding this comment.
thought: does this truncate index correctly
bluss
reviewed
Dec 31, 2017
| if entry_index >= found { | ||
| entry_index - 1 | ||
| } else { | ||
| entry_index | ||
| } |
There was a problem hiding this comment.
If found == 0; entry_index - 1 wraps around
|
Ok, to not bother you further, I've copied this branch for later reuse to this repo and I'll close the PR. :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I might be understanding the implementation wrong, but I think that order-preserving remove can be implemented by shifting both the
indicesand entryVec-s, then decrementing all indices after the removed one, then correcting the robin hood property. But maybe I'm wrong, please review carefully @bluss :)Because this operation is
O(n)compared to the stdO(1), I named itordered_removeso that you're forced to make a conscious decision to preserve order at the expense of performance.If this implementation makes sense, I'm happy to add a couple more commits to update the README and add this operation to
Entrytoo.