Add Vec::remove_insert()

[MoSal]

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.

[zeenix]

LGTM otherwise, thanks!

[zeenix]

• Can we have multiple unsafe blocks with each having as little scope as possible?
• Please adds Safety: comments above each use of unsafe, briefly describing how the invariants are being held in the block.

[MoSal]

Pushed a commit that does this (unsquashed, in case I overdid the explanations).

[sgued]

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 Vec and could be implemented on &mut [T], making it more generic.

[MoSal]

@sgued
I asked myself that actually.

Anyway, I relayed this to zulip. And slice methods rotate_left()/rotate_right() make this much simpler (commit incoming).

[MoSal]

I added this as a local trait implemented for AsMut<[T]> types in the project where I needed this and moved on.

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.

[zeenix]

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 (git reflog to find the commit, git reset --hard THE_COMMIT_HASH to revert to that and then git push --force)?

[MoSal]

@zeenix
Squashed.

[zeenix]

LGTM, thanks!

[zeenix]

I'm a bit baffled by the CI failure. I can't reproduce the warning locally. 🤔

[sgued]

This is a bug in 1.92 I think, you may not have the latest compiler.

[sgued]

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 core. As #635 (comment) points it can be implemented downstream too, so I'm not sure it should be merged as-is.

[sgued]

Hmm, no this does not reproduce for me either

[sgued]

This might be related: rust-lang/rust#147648

[scottmcm]

(Hi, I'm from the peanut gallery, having seen this in #t-libs > `remove_insert() for `Vec`, or more generally for `&mut [T]` @ 💬)

In patterns where an insertion directly follows a removal

Note that if it doesn't change the length then it'd be better as something on slice instead of on Vec.

That would also simplify the interface, because the slice could be sub-sliced [remove_index..=insert_index] and then the "move everything sideways one, filling in the hole with the argument and returning the thing that goes off the end" doesn't need to mention indexes at all.

(Some people might still define a local helper with the Ord::cmp to dispatch between them properly, but I bet often you know the relative positions)