Skip to content
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

A single method to rotate slices or vecs #65104

Open
STPR opened this issue Oct 4, 2019 · 5 comments
Open

A single method to rotate slices or vecs #65104

STPR opened this issue Oct 4, 2019 · 5 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@STPR
Copy link

STPR commented Oct 4, 2019

Hello,

I know there's already 2 methods to rotate slices or vec (rotate_right and rotate_left).
But they both accept unsigned numbers exclusively.

Why not a single method which could accept a signed number ?
A positive number would turn to the right and a negative one to the left

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 4, 2019
@AnthonyMikh
Copy link
Contributor

Could you provide a motivating example where having this method would greatly simplify the code? If you need it for your project, you can easilly roll your own implementaton using extension trait pattern:

trait SliceExt {
    fn rotate_signed(&mut self, rot: isize);
}

impl<T> SliceExt for [T] {
    fn rotate_signed(&mut self, rot: isize) {
        if rot >= 0 {
            self.rotate_right(rot as usize)
        } else {
            let (abs, overflowed) = rot.overflowing_abs();
            if overflowed {
                self.rotate_left(isize::max_value() as usize + 1)
            } else {
                self.rotate_left(abs as usize)
            }
        }
    }
}

@STPR
Copy link
Author

STPR commented Oct 4, 2019

Thanks for your code but it looks very complex to me.
Just looking at this makes me think it's a good reason to have it.

Having only 2 named methods to rotate brings some problems when you want to use both inside a function.
For example, if you want to make a function that needs to rotate right then left or the opposite, left then right depending on a parameter.

How are you going to program that with just rotate_right and rotate_left ?
It's going to be confusing.

With signed numbers we could use a tuple parameter like (4, -4) to rotate right then left or (-4, 4) to do the opposite.

@scottmcm
Copy link
Member

scottmcm commented Oct 5, 2019

You can rotate left or right with either method. s.rotate_left(mid) and s.rotate_right(s.len() - mid) do the same thing. Similarly, s.rotate_right(k) and s.rotate_left(s.len() - k) do the same thing.

If you want to rotate by a signed number, you can rotate by n.rem_euclid(s.len()) as usize.

(There was originally only one method, which also took usize, but that changed in #46777.)

@STPR
Copy link
Author

STPR commented Oct 5, 2019

I do understand why you have made rotate_left and rotate_right.
I still think that a single rotate method would be less confusing than:
"s.rotate_right(k) and s.rotate_left(s.len() - k) do the same thing"
or
n.rem_euclid(s.len()) as usize

> "Ruby’s Array.rotate shifts left-ward, Python’s deque.rotate shifts
right-ward. Both of their implementations allow passing negative numbers"

That's true, but at least they have a solution.
This is not a good justification to remove the single method.

> "remove ambiguity about direction"

Not true, see the above solutions.

> "alleviating need to read docs"

Reading the docs shall never be a problem and shall be recommended.
If a rotate method was correctly documented, you wouldn't see some peoples writing strange code like #65104 (comment)

> "make it easier for people who need to rotate right"

That's a joke ???

I tend to prefer the Python's way.
And even the Java's method "java.util.Collections.rotate()" do exactly like Python.

@STPR
Copy link
Author

STPR commented Oct 6, 2019

Let's say I want to rotate by -3 with your solution:

fn main() {
    let mut s: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    let n: i8 = -3;
    s.rotate_right(n.rem_euclid(s.len()) as usize);
    println!("{:?}", s);
}

Doesn't compile:

error[E0308]: mismatched types
 --> src\main.rs:4:33
  |
4 |     s.rotate_right(n.rem_euclid(s.len()) as usize);
  |                                 ^^^^^^^ expected i8, found usize
help: you can convert an `usize` to `i8` and panic if the converted value wouldn't fit
  |
4 |     s.rotate_right(n.rem_euclid(s.len().try_into().unwrap()) as usize);
  |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^

I ended up with:

fn main() {
    let mut s: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    let n: i8 = -3;
    s.rotate_right(n.rem_euclid(10) as usize);
    println!("{:?}", s);
}

Which works... But I think this could be clearer, less confusing, and probably faster, like this:

fn main() {
    let mut s: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
    let n: i8 = -3;
    s.rotate(n);
    println!("{:?}", s);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants