Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. #46777

frewsxcv · 2017-12-16T21:37:26Z

Background

Slices currently have an unstable rotate method which rotates
elements in the slice to the left N positions. Here is the
tracking issue for this unstable feature.

let mut a = ['a', 'b' ,'c', 'd', 'e', 'f'];
a.rotate(2);
assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']);

Proposal

Deprecate the rotate method and introduce rotate_left and
rotate_right methods.

let mut a = ['a', 'b' ,'c', 'd', 'e', 'f'];
a.rotate_left(2);
assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']);

let mut a = ['a', 'b' ,'c', 'd', 'e', 'f'];
a.rotate_right(2);
assert_eq!(a, ['e', 'f', 'a', 'b', 'c', 'd']);

Justification

I used this method today for my first time and (probably because I’m a
naive westerner who reads LTR) was surprised when the docs mentioned that
elements get rotated in a left-ward direction. I was in a situation
where I needed to shift elements in a right-ward direction and had to
context switch from the main problem I was working on and think how much
to rotate left in order to accomplish the right-ward rotation I needed.

Ruby’s Array.rotate shifts left-ward, Python’s deque.rotate shifts
right-ward. Both of their implementations allow passing negative numbers
to shift in the opposite direction respectively. The current rotate
implementation takes an unsigned integer argument which doesn't allow
the negative number behavior.

Introducing rotate_left and rotate_right would:

remove ambiguity about direction (alleviating need to read docs 😉)
make it easier for people who need to rotate right

rust-highfive · 2017-12-16T21:37:37Z

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

frewsxcv · 2017-12-16T21:44:04Z

If this needs to go through an RFC, let me know and I can throw one together 💁‍♂️

Also cc @scottmcm since you wrote the initial rotate implementation. Thank you soo much for writing [T]::rotate, I used it today for the latest Advent of Code problem ❤️

scottmcm

This seems good to me. The left/right is probably better since the takes a number (as opposed to some "position" concept, where rotating the two sides without mentioning a direction is the better API).

scottmcm · 2017-12-17T20:43:37Z

src/liballoc/slice.rs

+    ///
+    /// Takes linear (in `self.len()`) time.
+    ///
+    /// # Examples


Consider including an example that rotates a subslice, since that seems to be such a common confusion (see the recent changes to copy_from_slice and such).

scottmcm · 2017-12-18T06:02:29Z

src/liballoc/slice.rs

    /// ```
    #[unstable(feature = "slice_rotate", issue = "41891")]
-    pub fn rotate(&mut self, mid: usize) {
-        core_slice::SliceExt::rotate(self, mid);
+    pub fn rotate_right(&mut self, mid: usize) {


One more thought here: rotate (and thus rotate_left) used mid for the parameter because it has a useful interpretation as an index, the same as split_at (which uses the same parameter name).

rotate_right, however, doesn't have that same meaning, so it should have a different name. I propose k, the letter used in EoP (the book I was following while trying various possible implementations) and common in a quick search of specifically to-the-right rotation problems online.

frewsxcv · 2017-12-22T05:00:09Z

@scottmcm thanks for the feedback! both your comments have been addressed in the latest force push

scottmcm · 2017-12-22T19:05:51Z

Error looks non-transient, though weird:

[00:02:45]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[00:02:45] error: missing `fn`, `type`, or `const` for impl-item declaration
[00:02:45]     --> /checkout/src/liballoc/slice.rs:1437:33
[00:02:45]      |
[00:02:45] 1437 |       /// a[1..5].rotate_right(1);
[00:02:45]      |  _________________________________^
[00:02:45] 1438 | | l   /// assert_eq!(a, ['a', 'e', 'b', 'c', 'd', 'f']);
[00:02:45]      | |  missing `fn`, `type`, or `const`
[00:02:45] 
[00:02:48] error: aborting due to previous error
[00:02:48] 
[00:02:48] error: Could not compile `alloc`.

https://github.com/rust-lang/rust/pull/46777/files#diff-773807740b9d7f176c85b4e2e34b2607R1437

kennytm · 2017-12-22T19:09:22Z

src/liballoc/slice.rs

+    /// ```
+    /// let mut a = ['a', 'b', 'c', 'd', 'e', 'f'];
+    /// a[1..5].rotate_right(1);
+l   /// assert_eq!(a, ['a', 'e', 'b', 'c', 'd', 'f']);


There's an l at the start of the line 😄

Also, the code example is missing #![feature(slice_rotate)].

frewsxcv · 2017-12-23T02:42:35Z

ci errors fixed in the latest force push

scottmcm · 2017-12-23T02:55:39Z

src/libcore/slice/mod.rs

@@ -650,6 +653,16 @@ impl<T> SliceExt for [T] {
        }
    }

+    fn rotate_right(&mut self, mid: usize) {


nit: k here too.

good call! addressed in the latest force push

Background ========== Slices currently have an unstable [`rotate`] method which rotates elements in the slice to the _left_ N positions. [Here][tracking] is the tracking issue for this unstable feature. ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate(2); assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']); ``` Proposal ======== Deprecate the [`rotate`] method and introduce `rotate_left` and `rotate_right` methods. ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate_left(2); assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']); ``` ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate_right(2); assert_eq!(a, ['e', 'f', 'a', 'b', 'c', 'd']); ``` Justification ============= I used this method today for my first time and (probably because I’m a naive westerner who reads LTR) was surprised when the docs mentioned that elements get rotated in a left-ward direction. I was in a situation where I needed to shift elements in a right-ward direction and had to context switch from the main problem I was working on and think how much to rotate left in order to accomplish the right-ward rotation I needed. Ruby’s `Array.rotate` shifts left-ward, Python’s `deque.rotate` shifts right-ward. Both of their implementations allow passing negative numbers to shift in the opposite direction respectively. Introducing `rotate_left` and `rotate_right` would: - remove ambiguity about direction (alleviating need to read docs 😉) - make it easier for people who need to rotate right [`rotate`]: https://doc.rust-lang.org/std/primitive.slice.html#method.rotate [tracking]: rust-lang#41891

ollie27 · 2017-12-27T13:28:45Z

Are "left" and "right" the correct terms to use here or does this suffer from the same issue as trim_left and trim_right: #30459? Perhaps rotate_forwards and rotate_backwards would be better as slices have a concept of front and back but I don't think they really have a concept of left and right.

retep998 · 2017-12-27T14:33:27Z

Given that slices can be trivially represented as [a, b, c] it is very clear where the left is and where the right is. Forwards and backwards is much trickier to understand in my opinion.

ollie27 · 2017-12-27T16:15:48Z

VecDeque uses front and back in its method names. Ideally these rotate methods would be added to VecDeque as well so using left and right would seem a bit inconsistent there.

frewsxcv · 2017-12-27T16:49:06Z

Does 'forward' mean 'towards the back' or 'towards the front'? I could see arguments for either

frewsxcv · 2017-12-27T16:52:00Z

Okay, I'm embarrassed now since apparently the definition of 'forward' is literally "toward the front" in the dictionary. I guess that would means 'forward' is rotate-left?

scottmcm · 2017-12-27T20:05:06Z

@frewsxcv FWIW, I also was uncertain, perhaps since "iterating forward" is starts at the front and moves toward the back...

frewsxcv · 2018-01-02T02:43:04Z

anyone from @rust-lang/libs have thoughts about this?

mbrubeck · 2018-01-02T02:49:27Z

Regarding alternate names, there was previous discussion in #41891.

alexcrichton · 2018-01-10T02:24:51Z

@bors: r+

Thanks for the PR and thorough explanation @frewsxcv! We discussed this at @rust-lang/libs triage today and it got the green light from everyone.

bors · 2018-01-10T02:24:52Z

📌 Commit 66ef6b9 has been approved by alexcrichton

frewsxcv · 2018-01-10T03:26:52Z

@bors rollup

…chton Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. Background ========== Slices currently have an **unstable** [`rotate`] method which rotates elements in the slice to the _left_ N positions. [Here][tracking] is the tracking issue for this unstable feature. ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate(2); assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']); ``` Proposal ======== Deprecate the [`rotate`] method and introduce `rotate_left` and `rotate_right` methods. ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate_left(2); assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']); ``` ```rust let mut a = ['a', 'b' ,'c', 'd', 'e', 'f']; a.rotate_right(2); assert_eq!(a, ['e', 'f', 'a', 'b', 'c', 'd']); ``` Justification ============= I used this method today for my first time and (probably because I’m a naive westerner who reads LTR) was surprised when the docs mentioned that elements get rotated in a left-ward direction. I was in a situation where I needed to shift elements in a right-ward direction and had to context switch from the main problem I was working on and think how much to rotate left in order to accomplish the right-ward rotation I needed. Ruby’s `Array.rotate` shifts left-ward, Python’s `deque.rotate` shifts right-ward. Both of their implementations allow passing negative numbers to shift in the opposite direction respectively. The current `rotate` implementation takes an unsigned integer argument which doesn't allow the negative number behavior. Introducing `rotate_left` and `rotate_right` would: - remove ambiguity about direction (alleviating need to read docs 😉) - make it easier for people who need to rotate right [`rotate`]: https://doc.rust-lang.org/std/primitive.slice.html#method.rotate [tracking]: rust-lang#41891

Rollup of 5 pull requests - Successful merges: #46762, #46777, #47262, #47285, #47301 - Failed merges:

Restioson · 2018-01-17T13:11:51Z

Just a note, the docs still say rotate.

frewsxcv · 2018-01-17T13:30:45Z

the new methods haven't hit stable or beta yet, so youll need a nightly compiler and be looking at the nightly docs

https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.rotate_left

Restioson · 2018-01-17T13:32:14Z

Ah sorry, didn't notice that. I'd assumed that since the stable docs had nightly methods they'd be up to date with them.

frewsxcv · 2018-01-17T13:40:50Z

Stable docs do render unstable methods, but these new methods need to ride the release train from nightly to beta and stable before the methods show up in beta and stable docs

Restioson · 2018-01-17T13:42:20Z

Ah yes this makes sense, I forget that the thing blocking nightly features from stable is specifically feature gates. Thanks :)

The `rotate` method has been deprecated in favor of `rotate_left` and `rotate_right` since rust-lang/rust#46777, and will be deleted in rust-lang/rust#48450.

rust-highfive assigned BurntSushi Dec 16, 2017

frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 16, 2017

frewsxcv mentioned this pull request Dec 16, 2017

Tracking issue for slice_rotate #41891

Closed

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2017

frewsxcv force-pushed the frewsxcv-rotate branch from 0f58d20 to 9350d56 Compare December 17, 2017 19:50

scottmcm approved these changes Dec 17, 2017

View reviewed changes

scottmcm reviewed Dec 18, 2017

View reviewed changes

frewsxcv force-pushed the frewsxcv-rotate branch from 9350d56 to 80a9ac5 Compare December 22, 2017 04:59

kennytm reviewed Dec 22, 2017

View reviewed changes

frewsxcv force-pushed the frewsxcv-rotate branch from 80a9ac5 to 9cf9262 Compare December 23, 2017 00:55

scottmcm reviewed Dec 23, 2017

View reviewed changes

frewsxcv force-pushed the frewsxcv-rotate branch from 9cf9262 to 40d2003 Compare December 23, 2017 03:01

frewsxcv force-pushed the frewsxcv-rotate branch from 40d2003 to 66ef6b9 Compare December 25, 2017 07:01

alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2018

frewsxcv mentioned this pull request Jan 10, 2018

Rollup of 5 pull requests #47308

Merged

bors added a commit that referenced this pull request Jan 10, 2018

Auto merge of #47308 - frewsxcv:rollup, r=frewsxcv

92c32d2

Rollup of 5 pull requests - Successful merges: #46762, #46777, #47262, #47285, #47301 - Failed merges:

bors merged commit 66ef6b9 into rust-lang:master Jan 10, 2018

frewsxcv deleted the frewsxcv-rotate branch January 10, 2018 12:57

kennytm mentioned this pull request Feb 27, 2018

Call rotate_left instead of rotate. Manishearth/compiletest-rs#102

Merged

scottmcm mentioned this pull request Oct 5, 2019

A single method to rotate slices or vecs #65104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. #46777

Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. #46777

frewsxcv commented Dec 16, 2017 •

edited

Loading

rust-highfive commented Dec 16, 2017

frewsxcv commented Dec 16, 2017

scottmcm left a comment

scottmcm Dec 17, 2017

scottmcm Dec 18, 2017 •

edited

Loading

frewsxcv commented Dec 22, 2017

scottmcm commented Dec 22, 2017

kennytm Dec 22, 2017

frewsxcv commented Dec 23, 2017

scottmcm Dec 23, 2017

frewsxcv Dec 23, 2017

ollie27 commented Dec 27, 2017

retep998 commented Dec 27, 2017

ollie27 commented Dec 27, 2017

frewsxcv commented Dec 27, 2017

frewsxcv commented Dec 27, 2017

scottmcm commented Dec 27, 2017 •

edited

Loading

frewsxcv commented Jan 2, 2018

mbrubeck commented Jan 2, 2018

alexcrichton commented Jan 10, 2018

bors commented Jan 10, 2018

frewsxcv commented Jan 10, 2018

Restioson commented Jan 17, 2018

frewsxcv commented Jan 17, 2018

Restioson commented Jan 17, 2018

frewsxcv commented Jan 17, 2018 •

edited

Loading

Restioson commented Jan 17, 2018

Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. #46777

Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. #46777

Conversation

frewsxcv commented Dec 16, 2017 • edited Loading

Background

Proposal

Justification

rust-highfive commented Dec 16, 2017

frewsxcv commented Dec 16, 2017

scottmcm left a comment

Choose a reason for hiding this comment

scottmcm Dec 17, 2017

Choose a reason for hiding this comment

scottmcm Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

frewsxcv commented Dec 22, 2017

scottmcm commented Dec 22, 2017

kennytm Dec 22, 2017

Choose a reason for hiding this comment

frewsxcv commented Dec 23, 2017

scottmcm Dec 23, 2017

Choose a reason for hiding this comment

frewsxcv Dec 23, 2017

Choose a reason for hiding this comment

ollie27 commented Dec 27, 2017

retep998 commented Dec 27, 2017

ollie27 commented Dec 27, 2017

frewsxcv commented Dec 27, 2017

frewsxcv commented Dec 27, 2017

scottmcm commented Dec 27, 2017 • edited Loading

frewsxcv commented Jan 2, 2018

mbrubeck commented Jan 2, 2018

alexcrichton commented Jan 10, 2018

bors commented Jan 10, 2018

frewsxcv commented Jan 10, 2018

Restioson commented Jan 17, 2018

frewsxcv commented Jan 17, 2018

Restioson commented Jan 17, 2018

frewsxcv commented Jan 17, 2018 • edited Loading

Restioson commented Jan 17, 2018

frewsxcv commented Dec 16, 2017 •

edited

Loading

scottmcm Dec 18, 2017 •

edited

Loading

scottmcm commented Dec 27, 2017 •

edited

Loading

frewsxcv commented Jan 17, 2018 •

edited

Loading