Add Vec::remove_insert()
#635
Conversation
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 <CE.Mohammad.AlSaleh@gmail.com>
src/vec/mod.rs
Outdated
| assert!(remove_index < length); | ||
| assert!(insert_index < length); | ||
|
|
||
| unsafe { |
There was a problem hiding this comment.
- 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.
Pushed a commit that does this (unsquashed, in case I overdid the explanations).
Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>
|
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 |
| /// // only one element (2) is shifted back | ||
| /// assert_eq!(v, [0, 2, 4, 3]); | ||
| /// ``` | ||
| pub fn remove_insert(&mut self, remove_index: usize, insert_index: usize, element: T) -> T { |
There was a problem hiding this comment.
How about?
fn remove_insert(&mut self, remove_index: usize, insert_index: usize, element: T) -> T {
if remove_index < insert_index {
self[remove_index..=insert_index].rotate_left(1);
} else if insert_index < remove_index {
self[insert_index..=remove_index].rotate_right(1);
}
mem::replace(&mut self[insert_index], element)
}
There was a problem hiding this comment.
A benchmark would be welcome to compare both implementations. Given the complexity of the implementation of rotate_left and rotate_right, I'm convinced it would be slower and larger in code size.
There was a problem hiding this comment.
I wrote the comment below before seeing this conversation.
There was a problem hiding this comment.
A benchmark would be welcome to compare both implementations. Given the complexity of the implementation of rotate_left and rotate_right, I'm convinced it would be slower and larger in code size.
Yep. It's even slower that romove()+insert().
https://github.com/MoSal/bench-insert-remove
remove_insert_rotate u16 time: [42.533 ms 42.662 ms 42.813 ms]
remove and insert u16 time: [31.312 ms 31.379 ms 31.456 ms]
remove_insert_unsafe u16 time: [16.316 ms 16.383 ms 16.474 ms]
remove_insert_rotate u8 time: [55.221 ms 55.435 ms 55.695 ms]
remove and insert u8 time: [46.525 ms 46.745 ms 47.026 ms]
remove_insert_unsafe u8 time: [36.480 ms 36.582 ms 36.701 ms]
The test applies the Move-To-Front transform then the reverse on random u16 and u8 buffers. The u8 benches only differ in input buffer being larger, and memchr::memchr() being used instead of .iter().position().
Signed-off-by: Mohammad AlSaleh <CE.Mohammad.AlSaleh@gmail.com>
4 participants
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.