Remove panic from rotate_left and rotate_right?

language design
21

Well, if that was the proposal we could talk about that too. But I'd interpreted the request here as "always wraps in both debug and release and you can rely on that", which wouldn't help catch it in debug.

2 Likes
22

The calculation that results in -1 would have overflowed usize before you even get to rotate_left, and so it would panic in debug mode.

The playground example you posted panics in debug mode on usize overflow:

thread 'main' panicked at src/main.rs:7:9:
attempt to subtract with overflow
2 Likes
23

Ah, I see what you mean now. Fair.

I guess I'd still rather the panic for len+1 as well, since I'm not trying to do that, but it doesn't have the same semantic problems so it's less obvious whether that should just work.

I'd still prefer just adding the explicitly-wrapping version that takes isize, though. Because if wrapping is useful, I'd really like wrapping_rotate_left(-1) to actually be the same as wrapping_rotate_right(1).

24

I did some benchmark testing and found that there was no meaningful difference in time in replacing the std implementations with the following ones. I did find that the method using rem_euclid by @FZs was slower faster.

pub fn rotate_right(&mut self, k: usize) {
    if self.is_empty() {
        return;
    }
    let k = k % self.len();
    let mid = self.len() - k;
    let p = self.as_mut_ptr();

    unsafe {
        rotate::ptr_rotate(mid, p.add(mid), k);
    }
}    
pub fn rotate_left(&mut self, mid: usize) {
    if self.is_empty() {
        return;
    }
    let mid = mid % self.len();
    let k = self.len() - mid;
    let p = self.as_mut_ptr();

    unsafe {
        rotate::ptr_rotate(mid, p.add(mid), k);
    }
}

Perhaps these would be good candidates for rotate_left_wrapping and rotate_right_wrapping methods that would be used? I think it would be better to keep these as taking a usize and perhaps make a rotate_wrapping method that takes an isize such as the following

pub fn rotate_wrapping(&mut self, rot: isize) {
    if self.is_empty() {
        return;
    }
    self.rotate_right(rot.rem_euclid(self.len() as isize) as usize)
}

Edit: Turns out I'm not the best at benchmarking.

Edit2: I pushed my bechmark code here if people want to try it out. GitHub - spencer3035/rotate_test

25

A note on naming: it would be more consistent with the rest of std to use a prefix wrapping_ i.e. wrapping_rotate_right and wrapping_rotate_left, which is consistent with method names like wrapping_add, wrapping_sub, wrapping_mul, etc

See u8 - Rust

26

The words "wrapping" and "rotate" are essentially synonyms in this context though.

27

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.