The return of the GroupBy and GroupByMut iterators on slice

[Kerollmops]

According to rust-lang/rfcs#2477 (comment), I am opening this PR again, this time I implemented it in safe Rust only, it is therefore much easier to read and is completely safe.

This PR proposes to add two new methods to the slice, the group_by and group_by_mut. These two methods provide a way to iterate over non-overlapping sub-slices of a base slice that are separated by the predicate given by the user (e.g. Partial::eq, |a, b| a.abs() < b.abs()).

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);

An RFC was open 2 years ago but wasn't necessary.

[Kerollmops]

Hey @rylev,

Thanks for the comment, I changed the tests a little bit, however, do you have an idea on how I should fix this CI tidy issue? The file is now too long what should I do? Create a new one just for those two structs?

tidy error: /checkout/library/core/src/slice/iter.rs: too many lines (3133) (add // ignore-tidy-filelength to the file to suppress this error)

[rylev]

@Kerollmops I'm not sure. I'd also be curious to know what the right move is here (either to suppress the warning or to break up the file).

[lcnr]

I think it's fine to just suppress the warning for now. Splitting slice::iter into submodules is something that's easier in a followup PR

[lcnr]

cc @rust-lang/infra bors didn't assign anyone here

r? @m-ou-se

[camelid]

bors didn't assign anyone here

FYI I think you mean rust-highfive. Probably the bot was being redeployed or something.

[m-ou-se]

Thanks! This looks great!

2 years ago

Apologies again! We're working on streamlining our processes. This shouldn't happen again. :)

Create a new one just for those two structs?

We should probably split files like these into more submodules at some point (just like #78934 does). A separate file for these new structs would be good (e.g. slice/iter/group_by.rs). But leaving that for later is also fine. Then it can be done together with the rest of the file.

---

Only one thing left before we can merge this:

Can you open a tracking issue for this, and add its number in the unstable attributes? Thanks!

(By the way, feel free to rebase/squash commits.)

[Kerollmops]

I just created the tracking issue, added it to the files, and squashed everything.

No worries about the 2 years waiting time, I implemented it myself, it was quite fun! I also benchmarked it and the full safe Rust version is a little bit slower than the unsafe one.

[m-ou-se]

Thanks!

@bors r+

[bors]

📌 Commit 8b53be6 has been approved by m-ou-se

[SkiFire13]

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.

[m-ou-se]

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. :)

[SkiFire13]

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);

[Kerollmops]

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.

[SkiFire13]

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

[bors]

⌛ Testing commit 8b53be6 with merge b33e234...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing b33e234 to master...

[Kerollmops]

Thank you for the review @m-ou-se :)