From 66ef6b9c0995cc678a00f4d061ba8e6adb16f610 Mon Sep 17 00:00:00 2001 From: Corey Farwell Date: Sat, 16 Dec 2017 15:29:09 -0500 Subject: [PATCH] Deprecate [T]::rotate in favor of [T]::rotate_{left,right}. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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]: https://github.com/rust-lang/rust/issues/41891 --- src/liballoc/benches/slice.rs | 2 +- src/liballoc/slice.rs | 99 ++++++++++++++++++---------- src/liballoc/tests/slice.rs | 51 +++++++++++--- src/libcore/slice/mod.rs | 17 ++++- src/libcore/tests/slice.rs | 21 +++++- src/tools/compiletest/src/runtest.rs | 2 +- 6 files changed, 142 insertions(+), 50 deletions(-) diff --git a/src/liballoc/benches/slice.rs b/src/liballoc/benches/slice.rs index 17538d885f8..ee5182a1d46 100644 --- a/src/liballoc/benches/slice.rs +++ b/src/liballoc/benches/slice.rs @@ -343,7 +343,7 @@ macro_rules! rotate { fn $name(b: &mut Bencher) { let size = mem::size_of_val(&$gen(1)[0]); let mut v = $gen($len * 8 / size); - b.iter(|| black_box(&mut v).rotate(($mid*8+size-1)/size)); + b.iter(|| black_box(&mut v).rotate_left(($mid*8+size-1)/size)); b.bytes = (v.len() * size) as u64; } } diff --git a/src/liballoc/slice.rs b/src/liballoc/slice.rs index 18bb13d847a..06913de9fd8 100644 --- a/src/liballoc/slice.rs +++ b/src/liballoc/slice.rs @@ -1360,24 +1360,16 @@ impl [T] { core_slice::SliceExt::sort_unstable_by_key(self, f); } - /// Permutes the slice in-place such that `self[mid..]` moves to the - /// beginning of the slice while `self[..mid]` moves to the end of the - /// slice. Equivalently, rotates the slice `mid` places to the left - /// or `k = self.len() - mid` places to the right. - /// - /// This is a "k-rotation", a permutation in which item `i` moves to - /// position `i + k`, modulo the length of the slice. See _Elements - /// of Programming_ [§10.4][eop]. - /// - /// Rotation by `mid` and rotation by `k` are inverse operations. - /// - /// [eop]: https://books.google.com/books?id=CO9ULZGINlsC&pg=PA178&q=k-rotation + /// Rotates the slice in-place such that the first `mid` elements of the + /// slice move to the end while the last `self.len() - mid` elements move to + /// the front. After calling `rotate_left`, the element previously at index + /// `mid` will become the first element in the slice. /// /// # Panics /// /// This function will panic if `mid` is greater than the length of the - /// slice. (Note that `mid == self.len()` does _not_ panic; it's a nop - /// rotation with `k == 0`, the inverse of a rotation with `mid == 0`.) + /// slice. Note that `mid == self.len()` does _not_ panic and is a no-op + /// rotation. /// /// # Complexity /// @@ -1388,31 +1380,68 @@ impl [T] { /// ``` /// #![feature(slice_rotate)] /// - /// let mut a = [1, 2, 3, 4, 5, 6, 7]; - /// let mid = 2; - /// a.rotate(mid); - /// assert_eq!(&a, &[3, 4, 5, 6, 7, 1, 2]); - /// let k = a.len() - mid; - /// a.rotate(k); - /// assert_eq!(&a, &[1, 2, 3, 4, 5, 6, 7]); + /// let mut a = ['a', 'b', 'c', 'd', 'e', 'f']; + /// a.rotate_left(2); + /// assert_eq!(a, ['c', 'd', 'e', 'f', 'a', 'b']); + /// ``` /// - /// use std::ops::Range; - /// fn slide(slice: &mut [T], range: Range, to: usize) { - /// if to < range.start { - /// slice[to..range.end].rotate(range.start-to); - /// } else if to > range.end { - /// slice[range.start..to].rotate(range.end-range.start); - /// } - /// } - /// let mut v: Vec<_> = (0..10).collect(); - /// slide(&mut v, 1..4, 7); - /// assert_eq!(&v, &[0, 4, 5, 6, 1, 2, 3, 7, 8, 9]); - /// slide(&mut v, 6..8, 1); - /// assert_eq!(&v, &[0, 3, 7, 4, 5, 6, 1, 2, 8, 9]); + /// Rotating a subslice: + /// + /// ``` + /// #![feature(slice_rotate)] + /// + /// let mut a = ['a', 'b', 'c', 'd', 'e', 'f']; + /// a[1..5].rotate_left(1); + /// assert_eq!(a, ['a', 'c', 'd', 'e', 'b', 'f']); /// ``` #[unstable(feature = "slice_rotate", issue = "41891")] + pub fn rotate_left(&mut self, mid: usize) { + core_slice::SliceExt::rotate_left(self, mid); + } + + #[unstable(feature = "slice_rotate", issue = "41891")] + #[rustc_deprecated(since = "", reason = "renamed to `rotate_left`")] pub fn rotate(&mut self, mid: usize) { - core_slice::SliceExt::rotate(self, mid); + core_slice::SliceExt::rotate_left(self, mid); + } + + /// Rotates the slice in-place such that the first `self.len() - k` + /// elements of the slice move to the end while the last `k` elements move + /// to the front. After calling `rotate_right`, the element previously at + /// index `self.len() - k` will become the first element in the slice. + /// + /// # Panics + /// + /// This function will panic if `k` is greater than the length of the + /// slice. Note that `k == self.len()` does _not_ panic and is a no-op + /// rotation. + /// + /// # Complexity + /// + /// Takes linear (in `self.len()`) time. + /// + /// # Examples + /// + /// ``` + /// #![feature(slice_rotate)] + /// + /// let mut a = ['a', 'b', 'c', 'd', 'e', 'f']; + /// a.rotate_right(2); + /// assert_eq!(a, ['e', 'f', 'a', 'b', 'c', 'd']); + /// ``` + /// + /// Rotate a subslice: + /// + /// ``` + /// #![feature(slice_rotate)] + /// + /// let mut a = ['a', 'b', 'c', 'd', 'e', 'f']; + /// a[1..5].rotate_right(1); + /// assert_eq!(a, ['a', 'e', 'b', 'c', 'd', 'f']); + /// ``` + #[unstable(feature = "slice_rotate", issue = "41891")] + pub fn rotate_right(&mut self, k: usize) { + core_slice::SliceExt::rotate_right(self, k); } /// Copies the elements from `src` into `self`. diff --git a/src/liballoc/tests/slice.rs b/src/liballoc/tests/slice.rs index 85d5ce304b8..49bdc9e1b90 100644 --- a/src/liballoc/tests/slice.rs +++ b/src/liballoc/tests/slice.rs @@ -494,37 +494,72 @@ fn test_sort_stability() { } #[test] -fn test_rotate() { +fn test_rotate_left() { let expected: Vec<_> = (0..13).collect(); let mut v = Vec::new(); // no-ops v.clone_from(&expected); - v.rotate(0); + v.rotate_left(0); assert_eq!(v, expected); - v.rotate(expected.len()); + v.rotate_left(expected.len()); assert_eq!(v, expected); let mut zst_array = [(), (), ()]; - zst_array.rotate(2); + zst_array.rotate_left(2); // happy path v = (5..13).chain(0..5).collect(); - v.rotate(8); + v.rotate_left(8); assert_eq!(v, expected); let expected: Vec<_> = (0..1000).collect(); // small rotations in large slice, uses ptr::copy v = (2..1000).chain(0..2).collect(); - v.rotate(998); + v.rotate_left(998); assert_eq!(v, expected); v = (998..1000).chain(0..998).collect(); - v.rotate(2); + v.rotate_left(2); assert_eq!(v, expected); // non-small prime rotation, has a few rounds of swapping v = (389..1000).chain(0..389).collect(); - v.rotate(1000-389); + v.rotate_left(1000-389); + assert_eq!(v, expected); +} + +#[test] +fn test_rotate_right() { + let expected: Vec<_> = (0..13).collect(); + let mut v = Vec::new(); + + // no-ops + v.clone_from(&expected); + v.rotate_right(0); + assert_eq!(v, expected); + v.rotate_right(expected.len()); + assert_eq!(v, expected); + let mut zst_array = [(), (), ()]; + zst_array.rotate_right(2); + + // happy path + v = (5..13).chain(0..5).collect(); + v.rotate_right(5); + assert_eq!(v, expected); + + let expected: Vec<_> = (0..1000).collect(); + + // small rotations in large slice, uses ptr::copy + v = (2..1000).chain(0..2).collect(); + v.rotate_right(2); + assert_eq!(v, expected); + v = (998..1000).chain(0..998).collect(); + v.rotate_right(998); + assert_eq!(v, expected); + + // non-small prime rotation, has a few rounds of swapping + v = (389..1000).chain(0..389).collect(); + v.rotate_right(389); assert_eq!(v, expected); } diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index ca5cf04b1d4..fd91227343f 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -201,7 +201,10 @@ pub trait SliceExt { fn ends_with(&self, needle: &[Self::Item]) -> bool where Self::Item: PartialEq; #[unstable(feature = "slice_rotate", issue = "41891")] - fn rotate(&mut self, mid: usize); + fn rotate_left(&mut self, mid: usize); + + #[unstable(feature = "slice_rotate", issue = "41891")] + fn rotate_right(&mut self, k: usize); #[stable(feature = "clone_from_slice", since = "1.7.0")] fn clone_from_slice(&mut self, src: &[Self::Item]) where Self::Item: Clone; @@ -640,7 +643,7 @@ impl SliceExt for [T] { self.binary_search_by(|p| p.cmp(x)) } - fn rotate(&mut self, mid: usize) { + fn rotate_left(&mut self, mid: usize) { assert!(mid <= self.len()); let k = self.len() - mid; @@ -650,6 +653,16 @@ impl SliceExt for [T] { } } + fn rotate_right(&mut self, k: usize) { + assert!(k <= self.len()); + let mid = self.len() - k; + + unsafe { + let p = self.as_mut_ptr(); + rotate::ptr_rotate(mid, p.offset(mid as isize), k); + } + } + #[inline] fn clone_from_slice(&mut self, src: &[T]) where T: Clone { assert!(self.len() == src.len(), diff --git a/src/libcore/tests/slice.rs b/src/libcore/tests/slice.rs index fa4c2e9b373..2df7d6e60f6 100644 --- a/src/libcore/tests/slice.rs +++ b/src/libcore/tests/slice.rs @@ -290,17 +290,32 @@ fn test_iter_folds() { } #[test] -fn test_rotate() { +fn test_rotate_left() { const N: usize = 600; let a: &mut [_] = &mut [0; N]; for i in 0..N { a[i] = i; } - a.rotate(42); + a.rotate_left(42); let k = N - 42; for i in 0..N { - assert_eq!(a[(i+k)%N], i); + assert_eq!(a[(i + k) % N], i); + } +} + +#[test] +fn test_rotate_right() { + const N: usize = 600; + let a: &mut [_] = &mut [0; N]; + for i in 0..N { + a[i] = i; + } + + a.rotate_right(42); + + for i in 0..N { + assert_eq!(a[(i + 42) % N], i); } } diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 3aee88136a1..a82e282901f 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -2856,7 +2856,7 @@ fn read2_abbreviated(mut child: Child) -> io::Result { *skipped += data.len(); if data.len() <= TAIL_LEN { tail[..data.len()].copy_from_slice(data); - tail.rotate(data.len()); + tail.rotate_left(data.len()); } else { tail.copy_from_slice(&data[(data.len() - TAIL_LEN)..]); }