-
Notifications
You must be signed in to change notification settings - Fork 235
Add Vec::remove_insert()
#635
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
base: main
Are you sure you want to change the base?
Conversation
zeenix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise, thanks!
src/vec/mod.rs
Outdated
| assert!(remove_index < length); | ||
| assert!(insert_index < length); | ||
|
|
||
| unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we have multiple unsafe blocks with each having as little scope as possible?
- Please adds
Safety:comments above each use ofunsafe, briefly describing how the invariants are being held in the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit that does this (unsquashed, in case I overdid the explanations).
|
Do you know why this is not also available in the standard library's Vec? This would actually not even need to be implemented on |
|
I added this as a local trait implemented for Feel free to do with it whatever you want. The implementation using rotates is slower as noted a few days ago. So, only the original impl is actually useful performance wise. |
I understand that you yourself no longer need this but please consider the time reviewers put into this as well. It seems to me like all that's left now is reverting to the original implementation ( |
In patterns where an insertion directly follows a removal, this serves as being more efficient, since it only shifts/copies values as needed for the combined operation. This also comes with the added bonus of not needing to check for fullness, and thus, a `Result` is not needed as a return type. Signed-off-by: Mohammad AlSaleh <[email protected]>
|
@zeenix |
zeenix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
I'm a bit baffled by the CI failure. I can't reproduce the warning locally. 🤔 |
|
This is a bug in 1.92 I think, you may not have the latest compiler. |
|
Given that this is a feature that can be implemented on slices, I'm not fully certain it makes sense to merge it here? I'd rather try to get it implemented upstream in |
|
Hmm, no this does not reproduce for me either |
|
This might be related: rust-lang/rust#147648 |
In patterns where an insertion directly follows a removal, this serves
as being more efficient, since it only shifts/copies values as needed
for the combined operation.
This also comes with the added bonus of not needing to check for
fullness, and thus, a
Resultis not needed as a return type.