It seems unnessasry to me to have a panic for these functions for slices. Why can they not just wrap around as expected? I did a quick test of the implementations and checked everything worked.
In this case it would be more appropriate to add wrapped methods, like wrapping_rotate_left and wrapping_rotate_right, and maybe a checked variant as well (checked_rotate_left and checked_rotate_right), in analogy to wrapping_add and checked_add for integers in the stdlib.
Removing a documented panic case from standard library APIs because it "seems unnecessary" is a bit like removing the panic from the assert! macro because it "seems unnecessary". Surely, one can just remove that pescy panic assert!(false) produces, as the rest of its implementation doesn't need it, right? No. Users could have been relying on that (documented) panic to make their program safe against invalid states or adverserial inputs!
If you want modulo, you should call it yourself. % by a runtime value is surprisingly expensive -- far more than the easily predicted assert, on modern platforms -- so there's no reason to impose that cost on everyone doing the normal thing of just not making such a big index in the first place.
(Why do you even want to have it wrap? How'd you end up in that scenario?)
I think having them wrap around is mathematically reasonable; rotation is a modular operation, where if you rotate a list of N items N times individually, you get back the original list.
I also think the cost of % is still relevant, and additionally the presence of a rotation amount larger than the slice size could easily indicate a bug in a program that’s better complained about than glossed over. Adding % at the call site is possible; removing it from within the callee would only happen via compiler optimization if the rotation amount is a constant.
I find the expectation reasonable; integer rotations (of bits) do have that behaviour in Rust.
I don't quite buy the performance argument either: The panicking behaviour requires branching on n > slice.len() anyway, so making that branch instead compute the remainder and resume wouldn't hurt current code any more than removing an assert!(n <= slice.len()) would.
However, preserving documented stable behaviour seems more important. Are there any other examples where panics on some inputs have been removed later?
edit: Oh, slice rotates actually allow n == v.len() (behaving the same as n == 0), so a rotation not panicking does not even mean the argument is a valid index to the slice.
If you compute the rotation amount in such a way that you get -1 as usize, that will have a jump in the behaviour for any slice length that's not a power of two.
Besides that behavior in other languages, the term "rotate" would imply that it can be done indefinately. Especially that the rotation of vec.len() is valid seems to imply that as well.
It seems inconclusive if the modulo operation would result in more performance
loss than the assert. I may do some testing later on this.
I partially understand the argument that it should remain for stability reasons
(although I would disagree with someone who was relying on a panic rather than
proper error handling). But I also see value in making functions take the
largest reasonable range of inputs to make the user experience more
streamlined.
Perhaps an alternative compromise would be to make a rotate() function
similar to the following:
pub fn rotate(&mut self, rot: isize) {
if self.is_empty() {
return;
}
if rot >= 0 {
let rot = (rot % self.len()) as usize;
self.rotate_right(rot);
} else {
let rot = (rot.abs() % self.len()) as usize;
self.rotate_left(rot);
}
}
That way we get a function that does not panic and also has more generic
behavior. I think this is more elegant than making a wrapping version of both
existing rotate functions.
I think such a method would make sense, although your implementation is overcomplicated. Exactly because of the modular nature of rotations, one can express it with either rotate_left or rotate_right only, using Euclidean division:
pub fn rotate(&mut self, rot: isize) {
if self.is_empty() {
return;
}
self.rotate_right(rot.rem_euclid(self.len() as isize) as usize)
}
But why does it take a fencepost? "split" takes an index that points between two elements, so there are clearly len + 1 valid distinct inputs.
However, for rotate, there are only len distinct rotations to consider, so I would expect only len valid inputs (if wrap-around is not desired). The fact that slice.rotate accepts len + 1 different inputs does seem very inconsistent to me. Conceptually rotating by len is the shortest rotate that "wraps around", so I would expect rotating by len and by len + 1 to behave consistently -- either both go modulo len or neither does.
@scottmcm that seems like an implementation detail to me, given that the exposed interface is rotate_left/rotate_right.
It also covers a zero rotation if k is an unbounded integer. In fact the lemma becomes trivial then: the inverse of a k rotation is a -k rotation, and rotations that are equal modulo n are identical, so -k and n-k are the same.
Isn't "an index that points between two elements" a different, less precise, way to define a fencepost (since index 0 and len are not actually between two elements)?
This is a really elegant solution. I think it would be preferred to use the underlying call to rotate::ptr_rotate in order to avoid the assert in the original implmentations, but it would ideally use this method.
It is very often the case that if you overflow an integer:
it will panic in debug mode
you will get wrong answers in release mode
It's not specific to slice rotations. So it is not a convincing argument in favor of making slice rotation try to detect such overflows in its inputs post-factum.