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.

Sign up for GitHub

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

Add iterator specialisations for Repeat and Cycle #47370

Closed
wants to merge 3 commits into from
Closed

Add iterator specialisations for Repeat and Cycle #47370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6710669
Add finite iterator specialisations for Repeat
varkor Jan 11, 2018
d6e2867
Add finite iterator specialisations for Cycle
varkor Jan 12, 2018
71322b1
Fix issues with `all`
varkor Jan 12, 2018
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
39 changes: 38 additions & 1 deletion src/libcore/iter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@

#![stable(feature = "rust1", since = "1.0.0")]

use cmp;
use cmp::{self, Ordering};
use fmt;
use iter_private::TrustedRandomAccess;
use ops::Try;
Expand Down Expand Up @@ -643,6 +643,43 @@ impl<I> Iterator for Cycle<I> where I: Clone + Iterator {
_ => (usize::MAX, None)
}
}

#[inline]
fn all<F>(&mut self, f: F) -> bool where F: FnMut(Self::Item) -> bool {
self.iter.clone().chain(self.orig.clone()).all(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this short-circuits with an element inside self.orig, this iterator should continue with the next element after the matching one in self.orig. Currently it'll instead restart with the first element in self.orig.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's a good point. I'll fix that if the decision is to merge.

}

#[inline]
fn max(self) -> Option<Self::Item> where Self::Item: cmp::Ord { self.orig.clone().max() }

#[inline]
fn min(self) -> Option<Self::Item> where Self::Item: cmp::Ord {
cmp::min(self.iter.min(), self.orig.clone().min())
Copy link
Contributor

Choose a reason for hiding this comment

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

self.iter is a subset of self.orig, so the minimum of self.orig will be at least as small as the minimum of self.iter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah — this is necessary to preserve the consistency of the behaviour of min when multiple elements are equally minimum. It's necessary to return the first/leftmost one, which in this case may not be the same as the first one in self.orig.

}

#[inline]
fn max_by_key<B: Ord, F>(self, f: F) -> Option<Self::Item>
where F: FnMut(&Self::Item) -> B {
self.orig.clone().max_by_key(f)
}

#[inline]
fn max_by<F>(self, f: F) -> Option<Self::Item>
where F: FnMut(&Self::Item, &Self::Item) -> Ordering {
self.orig.clone().max_by(f)
}

#[inline]
fn min_by_key<B: Ord, F>(self, f: F) -> Option<Self::Item>
where F: FnMut(&Self::Item) -> B {
self.iter.chain(self.orig.clone()).min_by_key(f)
}

#[inline]
fn min_by<F>(self, f: F) -> Option<Self::Item>
where F: FnMut(&Self::Item, &Self::Item) -> Ordering {
self.iter.chain(self.orig.clone()).min_by(f)
}
}

#[unstable(feature = "fused", issue = "35602")]
Expand Down
34 changes: 34 additions & 0 deletions src/libcore/iter/sources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use fmt;
use marker;
use usize;
use cmp::Ordering;

use super::{FusedIterator, TrustedLen};

Expand All @@ -31,8 +32,41 @@ impl<A: Clone> Iterator for Repeat<A> {

#[inline]
fn next(&mut self) -> Option<A> { Some(self.element.clone()) }

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) { (usize::MAX, None) }

#[inline]
fn nth(&mut self, _: usize) -> Option<A> { self.next() }
Copy link
Member

Choose a reason for hiding this comment

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

I like this one more than the others here, but it's an observable change if A::clone has side-effects. Is that allowable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't consider that. It seems to me that A::clone is an implementation detail that shouldn't be relied upon, but as it's technically observable at the moment, and the documentation doesn't specify otherwise, it might be worth clarifying this.


#[inline]
fn all<F>(&mut self, mut f: F) -> bool where F: FnMut(A) -> bool { f(self.element.clone()) }

#[inline]
fn max(mut self) -> Option<A> { self.next() }

#[inline]
fn min(mut self) -> Option<A> { self.next() }

#[inline]
fn max_by_key<B: Ord, F>(mut self, _: F) -> Option<A> where F: FnMut(&A) -> B {
self.next()
}

#[inline]
fn max_by<F>(mut self, _: F) -> Option<A> where F: FnMut(&A, &A) -> Ordering {
self.next()
}

#[inline]
fn min_by_key<B: Ord, F>(mut self, _: F) -> Option<A> where F: FnMut(&A) -> B {
self.next()
}

#[inline]
fn min_by<F>(mut self, _: F) -> Option<A> where F: FnMut(&A, &A) -> Ordering {
self.next()
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
28 changes: 28 additions & 0 deletions src/libcore/tests/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use core::iter::*;
use core::{i8, i16, isize};
use core::usize;
use std::cmp::Ordering;

#[test]
fn test_lt() {
Expand Down Expand Up @@ -866,6 +867,20 @@ fn test_cycle() {
assert_eq!(it.next(), None);
}

#[test]
fn test_cycle_iterator() {
let a = 1;
let b = 5;
let mut it = (a..b).cycle();
assert_eq!(it.all(|x| x <= b), true);
assert_eq!((a..b).cycle().max(), Some(b - 1));
assert_eq!((a..b).cycle().min(), Some(a));
assert_eq!((a..b).cycle().max_by_key(|x| -x), Some(a));
assert_eq!((a..b).cycle().max_by(|x, y| y.cmp(x)), Some(a));
assert_eq!((a..b).cycle().min_by_key(|x| -x), Some(b - 1));
assert_eq!((a..b).cycle().min_by(|x, y| y.cmp(x)), Some(b - 1));
}

#[test]
fn test_iterator_nth() {
let v: &[_] = &[0, 1, 2, 3, 4];
Expand Down Expand Up @@ -1405,6 +1420,19 @@ fn test_repeat() {
assert_eq!(it.next(), Some(42));
}

#[test]
fn test_repeat_iterator() {
let mut it = repeat(42);
assert_eq!(it.nth(usize::MAX), Some(42));
assert_eq!(it.all(|x| x == 42), true);
assert_eq!(repeat(42).max(), Some(42));
assert_eq!(repeat(42).min(), Some(42));
assert_eq!(repeat(42).max_by_key(|_| 0), Some(42));
assert_eq!(repeat(42).max_by(|_, _| Ordering::Equal), Some(42));
assert_eq!(repeat(42).min_by_key(|_| 0), Some(42));
assert_eq!(repeat(42).min_by(|_, _| Ordering::Equal), Some(42));
}

#[test]
fn test_fuse() {
let mut it = 0..3;
Expand Down