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

The return of the GroupBy and GroupByMut iterators on slice #79895

Merged
merged 14 commits into from
Dec 31, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
#![feature(try_trait)]
#![feature(type_alias_impl_trait)]
#![feature(associated_type_bounds)]
#![feature(slice_group_by)]
// Allow testing this library

#[cfg(test)]
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ pub use core::slice::{Chunks, Windows};
pub use core::slice::{ChunksExact, ChunksExactMut};
#[stable(feature = "rust1", since = "1.0.0")]
pub use core::slice::{ChunksMut, Split, SplitMut};
#[unstable(feature = "slice_group_by", issue = "none")]
pub use core::slice::{GroupBy, GroupByMut};
#[stable(feature = "rust1", since = "1.0.0")]
pub use core::slice::{Iter, IterMut};
#[stable(feature = "rchunks", since = "1.31.0")]
Expand Down
1 change: 1 addition & 0 deletions library/alloc/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#![feature(iter_map_while)]
#![feature(int_bits_const)]
#![feature(vecdeque_binary_search)]
#![feature(slice_group_by)]

use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};
Expand Down
58 changes: 58 additions & 0 deletions library/alloc/tests/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1898,3 +1898,61 @@ fn subslice_patterns() {
m!(&mut v, [..] => ());
m!(&mut v, [x, .., y] => c!((x, y), (&mut N, &mut N), (&mut N(0), &mut N(4))));
}

#[test]
fn test_group_by() {
Kerollmops marked this conversation as resolved.
Show resolved Hide resolved
let slice = &[1, 1, 1, 3, 3, 2, 2, 2, 1, 0];

let mut iter = slice.group_by(|a, b| a == b);
assert_eq!(iter.next(), Some(&[1, 1, 1][..]));
assert_eq!(iter.next(), Some(&[3, 3][..]));
assert_eq!(iter.next(), Some(&[2, 2, 2][..]));
assert_eq!(iter.next(), Some(&[1][..]));
assert_eq!(iter.next(), Some(&[0][..]));
assert_eq!(iter.next(), None);

let mut iter = slice.group_by(|a, b| a == b);
assert_eq!(iter.next_back(), Some(&[0][..]));
assert_eq!(iter.next_back(), Some(&[1][..]));
assert_eq!(iter.next_back(), Some(&[2, 2, 2][..]));
assert_eq!(iter.next_back(), Some(&[3, 3][..]));
assert_eq!(iter.next_back(), Some(&[1, 1, 1][..]));
assert_eq!(iter.next_back(), None);

let mut iter = slice.group_by(|a, b| a == b);
assert_eq!(iter.next(), Some(&[1, 1, 1][..]));
assert_eq!(iter.next_back(), Some(&[0][..]));
assert_eq!(iter.next(), Some(&[3, 3][..]));
assert_eq!(iter.next_back(), Some(&[1][..]));
assert_eq!(iter.next(), Some(&[2, 2, 2][..]));
assert_eq!(iter.next_back(), None);
}

#[test]
fn test_group_by_mut() {
let slice = &mut [1, 1, 1, 3, 3, 2, 2, 2, 1, 0];

let mut iter = slice.group_by_mut(|a, b| a == b);
assert_eq!(iter.next(), Some(&mut [1, 1, 1][..]));
assert_eq!(iter.next(), Some(&mut [3, 3][..]));
assert_eq!(iter.next(), Some(&mut [2, 2, 2][..]));
assert_eq!(iter.next(), Some(&mut [1][..]));
assert_eq!(iter.next(), Some(&mut [0][..]));
assert_eq!(iter.next(), None);

let mut iter = slice.group_by_mut(|a, b| a == b);
assert_eq!(iter.next_back(), Some(&mut [0][..]));
assert_eq!(iter.next_back(), Some(&mut [1][..]));
assert_eq!(iter.next_back(), Some(&mut [2, 2, 2][..]));
assert_eq!(iter.next_back(), Some(&mut [3, 3][..]));
assert_eq!(iter.next_back(), Some(&mut [1, 1, 1][..]));
assert_eq!(iter.next_back(), None);

let mut iter = slice.group_by_mut(|a, b| a == b);
assert_eq!(iter.next(), Some(&mut [1, 1, 1][..]));
assert_eq!(iter.next_back(), Some(&mut [0][..]));
assert_eq!(iter.next(), Some(&mut [3, 3][..]));
assert_eq!(iter.next_back(), Some(&mut [1][..]));
assert_eq!(iter.next(), Some(&mut [2, 2, 2][..]));
assert_eq!(iter.next_back(), None);
}
174 changes: 174 additions & 0 deletions library/core/src/slice/iter.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// ignore-tidy-filelength
//! Definitions of a bunch of iterators for `[T]`.

#[macro_use] // import iterator! and forward_iterator!
Expand Down Expand Up @@ -2967,3 +2968,176 @@ unsafe impl<'a, T> TrustedRandomAccess for IterMut<'a, T> {
false
}
}

/// An iterator over slice in (non-overlapping) chunks separated by a predicate.
///
/// This struct is created by the [`group_by`] method on [slices].
///
/// [`group_by`]: ../../std/primitive.slice.html#method.group_by
/// [slices]: ../../std/primitive.slice.html
#[unstable(feature = "slice_group_by", issue = "none")]
pub struct GroupBy<'a, T: 'a, P> {
slice: &'a [T],
predicate: P,
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> GroupBy<'a, T, P> {
pub(super) fn new(slice: &'a [T], predicate: P) -> Self {
GroupBy { slice, predicate }
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> Iterator for GroupBy<'a, T, P>
where
P: FnMut(&T, &T) -> bool,
{
type Item = &'a [T];

#[inline]
fn next(&mut self) -> Option<Self::Item> {
if self.slice.is_empty() {
None
} else {
let mut len = 1;
let mut iter = self.slice.windows(2);
while let Some([l, r]) = iter.next() {
if (self.predicate)(l, r) { len += 1 } else { break }
}
let (head, tail) = self.slice.split_at(len);
self.slice = tail;
Some(head)
}
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
if self.slice.is_empty() { (0, Some(0)) } else { (1, Some(self.slice.len())) }
}

#[inline]
fn last(mut self) -> Option<Self::Item> {
self.next_back()
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> DoubleEndedIterator for GroupBy<'a, T, P>
where
P: FnMut(&T, &T) -> bool,
{
#[inline]
fn next_back(&mut self) -> Option<Self::Item> {
if self.slice.is_empty() {
None
} else {
let mut len = 1;
let mut iter = self.slice.windows(2);
while let Some([l, r]) = iter.next_back() {
if (self.predicate)(l, r) { len += 1 } else { break }
}
let (head, tail) = self.slice.split_at(self.slice.len() - len);
self.slice = head;
Some(tail)
}
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> FusedIterator for GroupBy<'a, T, P> where P: FnMut(&T, &T) -> bool {}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a + fmt::Debug, P> fmt::Debug for GroupBy<'a, T, P> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("GroupBy").field("slice", &self.slice).finish()
}
}

/// An iterator over slice in (non-overlapping) mutable chunks separated
/// by a predicate.
///
/// This struct is created by the [`group_by_mut`] method on [slices].
///
/// [`group_by_mut`]: ../../std/primitive.slice.html#method.group_by_mut
/// [slices]: ../../std/primitive.slice.html
#[unstable(feature = "slice_group_by", issue = "none")]
pub struct GroupByMut<'a, T: 'a, P> {
slice: &'a mut [T],
predicate: P,
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> GroupByMut<'a, T, P> {
pub(super) fn new(slice: &'a mut [T], predicate: P) -> Self {
GroupByMut { slice, predicate }
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> Iterator for GroupByMut<'a, T, P>
where
P: FnMut(&T, &T) -> bool,
{
type Item = &'a mut [T];

#[inline]
fn next(&mut self) -> Option<Self::Item> {
if self.slice.is_empty() {
None
} else {
let mut len = 1;
let mut iter = self.slice.windows(2);
while let Some([l, r]) = iter.next() {
if (self.predicate)(l, r) { len += 1 } else { break }
}
let slice = mem::take(&mut self.slice);
let (head, tail) = slice.split_at_mut(len);
self.slice = tail;
Some(head)
}
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
if self.slice.is_empty() { (0, Some(0)) } else { (1, Some(self.slice.len())) }
}

#[inline]
fn last(mut self) -> Option<Self::Item> {
self.next_back()
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> DoubleEndedIterator for GroupByMut<'a, T, P>
where
P: FnMut(&T, &T) -> bool,
{
#[inline]
fn next_back(&mut self) -> Option<Self::Item> {
if self.slice.is_empty() {
None
} else {
let mut len = 1;
let mut iter = self.slice.windows(2);
while let Some([l, r]) = iter.next_back() {
if (self.predicate)(l, r) { len += 1 } else { break }
}
let slice = mem::take(&mut self.slice);
let (head, tail) = slice.split_at_mut(slice.len() - len);
self.slice = head;
Some(tail)
}
}
}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a, P> FusedIterator for GroupByMut<'a, T, P> where P: FnMut(&T, &T) -> bool {}

#[unstable(feature = "slice_group_by", issue = "none")]
impl<'a, T: 'a + fmt::Debug, P> fmt::Debug for GroupByMut<'a, T, P> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("GroupByMut").field("slice", &self.slice).finish()
}
}
63 changes: 63 additions & 0 deletions library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ pub use iter::{ArrayChunks, ArrayChunksMut};
#[unstable(feature = "array_windows", issue = "75027")]
pub use iter::ArrayWindows;

#[unstable(feature = "slice_group_by", issue = "none")]
pub use iter::{GroupBy, GroupByMut};

#[unstable(feature = "split_inclusive", issue = "72360")]
pub use iter::{SplitInclusive, SplitInclusiveMut};

Expand Down Expand Up @@ -1207,6 +1210,66 @@ impl<T> [T] {
RChunksExactMut::new(self, chunk_size)
}

/// Returns an iterator over the slice producing non-overlapping runs
/// of elements using the predicate to separate them.
Comment on lines +1213 to +1214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should have a stronger mark on the fact that it only groups consecutive items, especially with such an ambiguous function name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say 'runs of elements' already implies that. But I'd be happy to review your PR if you have an idea on how to improve the wording. Improvements to documentation are always welcome. :)

///
/// The predicate is called on two elements following themselves,
/// it means the predicate is called on `slice[0]` and `slice[1]`
/// then on `slice[1]` and `slice[2]` and so on.
///
/// # Examples
///
/// ```
/// #![feature(slice_group_by)]
///
/// let slice = &[1, 1, 1, 3, 3, 2, 2, 2];
///
/// let mut iter = slice.group_by(|a, b| a == b);
///
/// assert_eq!(iter.next(), Some(&[1, 1, 1][..]));
/// assert_eq!(iter.next(), Some(&[3, 3][..]));
/// assert_eq!(iter.next(), Some(&[2, 2, 2][..]));
/// assert_eq!(iter.next(), None);
Comment on lines +1225 to +1232
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this gives a false perception that it groups the same items, no matter where they are. Would probably be better if the same element appeared multiple times in different groups, for example:

Suggested change
/// let slice = &[1, 1, 1, 3, 3, 2, 2, 2];
///
/// let mut iter = slice.group_by(|a, b| a == b);
///
/// assert_eq!(iter.next(), Some(&[1, 1, 1][..]));
/// assert_eq!(iter.next(), Some(&[3, 3][..]));
/// assert_eq!(iter.next(), Some(&[2, 2, 2][..]));
/// assert_eq!(iter.next(), None);
/// let slice = &[1, 1, 1, 3, 3, 2, 2, 2, 1, 1];
///
/// let mut iter = slice.group_by(|a, b| a == b);
///
/// assert_eq!(iter.next(), Some(&[1, 1, 1][..]));
/// assert_eq!(iter.next(), Some(&[3, 3][..]));
/// assert_eq!(iter.next(), Some(&[2, 2, 2][..]));
/// assert_eq!(iter.next(), Some(&[1, 1][..]));
/// assert_eq!(iter.next(), None);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point but I find the second example clear enough and let the user understand that the split are based on the specified function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if someone skips the second example? You could argue that's the user's fault, but I'd like to make this as much foolproof as we can

/// ```
Kerollmops marked this conversation as resolved.
Show resolved Hide resolved
#[unstable(feature = "slice_group_by", issue = "none")]
#[inline]
pub fn group_by<F>(&self, pred: F) -> GroupBy<'_, T, F>
where
F: FnMut(&T, &T) -> bool,
{
GroupBy::new(self, pred)
}

/// Returns an iterator over the slice producing non-overlapping mutable
/// runs of elements using the predicate to separate them.
///
/// The predicate is called on two elements following themselves,
/// it means the predicate is called on `slice[0]` and `slice[1]`
/// then on `slice[1]` and `slice[2]` and so on.
///
/// # Examples
///
/// ```
/// #![feature(slice_group_by)]
///
/// let slice = &mut [1, 1, 1, 3, 3, 2, 2, 2];
///
/// let mut iter = slice.group_by_mut(|a, b| a == b);
///
/// assert_eq!(iter.next(), Some(&mut [1, 1, 1][..]));
/// assert_eq!(iter.next(), Some(&mut [3, 3][..]));
/// assert_eq!(iter.next(), Some(&mut [2, 2, 2][..]));
/// assert_eq!(iter.next(), None);
/// ```
#[unstable(feature = "slice_group_by", issue = "none")]
#[inline]
pub fn group_by_mut<F>(&mut self, pred: F) -> GroupByMut<'_, T, F>
where
F: FnMut(&T, &T) -> bool,
{
GroupByMut::new(self, pred)
}

/// Divides one slice into two at an index.
///
/// The first will contain all indices from `[0, mid)` (excluding
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
#![feature(nonzero_leading_trailing_zeros)]
#![feature(const_option)]
#![feature(integer_atomics)]
#![feature(slice_group_by)]
#![deny(unsafe_op_in_unsafe_fn)]

extern crate test;
Expand Down