From 8d18e57b8a957bee7de4586eb1aeecac0ed718d3 Mon Sep 17 00:00:00 2001
From: Tim Vermeulen <tvermeulen@me.com>
Date: Tue, 12 Mar 2019 17:52:10 +0100
Subject: [PATCH 1/5] Fix the bench_max and bench_max_by_key benchmarks
---
src/libcore/benches/iter.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libcore/benches/iter.rs b/src/libcore/benches/iter.rs
index 1dd2bd3ee78aa..825bd368bdf1d 100644
--- a/src/libcore/benches/iter.rs
+++ b/src/libcore/benches/iter.rs
@@ -35,7 +35,7 @@ fn scatter(x: i32) -> i32 { (x * 31) % 127 }
fn bench_max_by_key(b: &mut Bencher) {
b.iter(|| {
let it = 0..100;
- it.max_by_key(|&x| scatter(x))
+ it.map(black_box).max_by_key(|&x| scatter(x))
})
}
@@ -56,7 +56,7 @@ fn bench_max_by_key2(b: &mut Bencher) {
fn bench_max(b: &mut Bencher) {
b.iter(|| {
let it = 0..100;
- it.map(scatter).max()
+ it.map(black_box).map(scatter).max()
})
}
From b23a0473b366134ffa792fc52f538c0577a3a397 Mon Sep 17 00:00:00 2001
From: Tim Vermeulen <tvermeulen@me.com>
Date: Tue, 12 Mar 2019 17:52:26 +0100
Subject: [PATCH 2/5] Remove the projection part of select_fold1
---
src/libcore/iter/traits/iterator.rs | 77 ++++++++---------------------
1 file changed, 21 insertions(+), 56 deletions(-)
diff --git a/src/libcore/iter/traits/iterator.rs b/src/libcore/iter/traits/iterator.rs
index 861e9c3157a79..199c6cdfa48be 100644
--- a/src/libcore/iter/traits/iterator.rs
+++ b/src/libcore/iter/traits/iterator.rs
@@ -2008,12 +2008,8 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn max(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
- select_fold1(self,
- |_| (),
- // switch to y even if it is only equal, to preserve
- // stability.
- |_, x, _, y| *x <= *y)
- .map(|(_, x)| x)
+ // switch to y even if it is only equal, to preserve stability.
+ select_fold1(self, |x, y| x <= y)
}
/// Returns the minimum element of an iterator.
@@ -2038,12 +2034,8 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn min(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
- select_fold1(self,
- |_| (),
- // only switch to y if it is strictly smaller, to
- // preserve stability.
- |_, x, _, y| *x > *y)
- .map(|(_, x)| x)
+ // only switch to y if it is strictly smaller, to preserve stability.
+ select_fold1(self, |x, y| x > y)
}
/// Returns the element that gives the maximum value from the
@@ -2062,15 +2054,11 @@ pub trait Iterator {
/// ```
#[inline]
#[stable(feature = "iter_cmp_by_key", since = "1.6.0")]
- fn max_by_key<B: Ord, F>(self, f: F) -> Option<Self::Item>
+ fn max_by_key<B: Ord, F>(self, mut f: F) -> Option<Self::Item>
where Self: Sized, F: FnMut(&Self::Item) -> B,
{
- select_fold1(self,
- f,
- // switch to y even if it is only equal, to preserve
- // stability.
- |x_p, _, y_p, _| x_p <= y_p)
- .map(|(_, x)| x)
+ // switch to y even if it is only equal, to preserve stability.
+ select_fold1(self.map(|x| (f(&x), x)), |(x_p, _), (y_p, _)| x_p <= y_p).map(|(_, x)| x)
}
/// Returns the element that gives the maximum value with respect to the
@@ -2092,12 +2080,8 @@ pub trait Iterator {
fn max_by<F>(self, mut compare: F) -> Option<Self::Item>
where Self: Sized, F: FnMut(&Self::Item, &Self::Item) -> Ordering,
{
- select_fold1(self,
- |_| (),
- // switch to y even if it is only equal, to preserve
- // stability.
- |_, x, _, y| Ordering::Greater != compare(x, y))
- .map(|(_, x)| x)
+ // switch to y even if it is only equal, to preserve stability.
+ select_fold1(self, |x, y| compare(x, y) != Ordering::Greater)
}
/// Returns the element that gives the minimum value from the
@@ -2115,15 +2099,11 @@ pub trait Iterator {
/// assert_eq!(*a.iter().min_by_key(|x| x.abs()).unwrap(), 0);
/// ```
#[stable(feature = "iter_cmp_by_key", since = "1.6.0")]
- fn min_by_key<B: Ord, F>(self, f: F) -> Option<Self::Item>
+ fn min_by_key<B: Ord, F>(self, mut f: F) -> Option<Self::Item>
where Self: Sized, F: FnMut(&Self::Item) -> B,
{
- select_fold1(self,
- f,
- // only switch to y if it is strictly smaller, to
- // preserve stability.
- |x_p, _, y_p, _| x_p > y_p)
- .map(|(_, x)| x)
+ // only switch to y if it is strictly smaller, to preserve stability.
+ select_fold1(self.map(|x| (f(&x), x)), |(x_p, _), (y_p, _)| x_p > y_p).map(|(_, x)| x)
}
/// Returns the element that gives the minimum value with respect to the
@@ -2145,12 +2125,8 @@ pub trait Iterator {
fn min_by<F>(self, mut compare: F) -> Option<Self::Item>
where Self: Sized, F: FnMut(&Self::Item, &Self::Item) -> Ordering,
{
- select_fold1(self,
- |_| (),
- // switch to y even if it is strictly smaller, to
- // preserve stability.
- |_, x, _, y| Ordering::Greater == compare(x, y))
- .map(|(_, x)| x)
+ // switch to y even if it is strictly smaller, to preserve stability.
+ select_fold1(self, |x, y| compare(x, y) == Ordering::Greater)
}
@@ -2693,34 +2669,23 @@ pub trait Iterator {
}
}
-/// Select an element from an iterator based on the given "projection"
-/// and "comparison" function.
+/// Select an element from an iterator based on the given "comparison"
+/// function.
///
/// This is an idiosyncratic helper to try to factor out the
/// commonalities of {max,min}{,_by}. In particular, this avoids
/// having to implement optimizations several times.
#[inline]
-fn select_fold1<I, B, FProj, FCmp>(mut it: I,
- mut f_proj: FProj,
- mut f_cmp: FCmp) -> Option<(B, I::Item)>
- where I: Iterator,
- FProj: FnMut(&I::Item) -> B,
- FCmp: FnMut(&B, &I::Item, &B, &I::Item) -> bool
+fn select_fold1<I, F>(mut it: I, mut f: F) -> Option<I::Item>
+ where
+ I: Iterator,
+ F: FnMut(&I::Item, &I::Item) -> bool,
{
// start with the first element as our selection. This avoids
// having to use `Option`s inside the loop, translating to a
// sizeable performance gain (6x in one case).
it.next().map(|first| {
- let first_p = f_proj(&first);
-
- it.fold((first_p, first), |(sel_p, sel), x| {
- let x_p = f_proj(&x);
- if f_cmp(&sel_p, &sel, &x_p, &x) {
- (x_p, x)
- } else {
- (sel_p, sel)
- }
- })
+ it.fold(first, |sel, x| if f(&sel, &x) { x } else { sel })
})
}
From 0de63d901b8ec550179b17ea06756e679b9fd461 Mon Sep 17 00:00:00 2001
From: Tim Vermeulen <tvermeulen@me.com>
Date: Tue, 12 Mar 2019 17:53:25 +0100
Subject: [PATCH 3/5] Fix comment
---
src/libcore/iter/traits/iterator.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libcore/iter/traits/iterator.rs b/src/libcore/iter/traits/iterator.rs
index 199c6cdfa48be..980de229fa661 100644
--- a/src/libcore/iter/traits/iterator.rs
+++ b/src/libcore/iter/traits/iterator.rs
@@ -2125,7 +2125,7 @@ pub trait Iterator {
fn min_by<F>(self, mut compare: F) -> Option<Self::Item>
where Self: Sized, F: FnMut(&Self::Item, &Self::Item) -> Ordering,
{
- // switch to y even if it is strictly smaller, to preserve stability.
+ // only switch to y if it is strictly smaller, to preserve stability.
select_fold1(self, |x, y| compare(x, y) == Ordering::Greater)
}
From 18192505568ce4c58995ca69652eaf088b17345b Mon Sep 17 00:00:00 2001
From: Tim Vermeulen <tvermeulen@me.com>
Date: Tue, 12 Mar 2019 19:25:44 +0100
Subject: [PATCH 4/5] Add tests to ensure that Iterator::min and Iterator::max
are stable
---
src/libcore/tests/iter.rs | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs
index d880abb181c20..a9db9b35d8d80 100644
--- a/src/libcore/tests/iter.rs
+++ b/src/libcore/tests/iter.rs
@@ -1082,12 +1082,39 @@ fn test_iterator_product_result() {
assert_eq!(v.iter().cloned().product::<Result<i32, _>>(), Err(()));
}
+/// A wrapper struct that implements `Eq` and `Ord` based on the wrapped
+/// integer modulo 3. Used to test that `Iterator::max` and `Iterator::min`
+/// return the correct element if some of them are equal.
+#[derive(Debug)]
+struct Mod3(i32);
+
+impl PartialEq for Mod3 {
+ fn eq(&self, other: &Self) -> bool {
+ self.0 % 3 == other.0 % 3
+ }
+}
+
+impl Eq for Mod3 {}
+
+impl PartialOrd for Mod3 {
+ fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+ Some(self.cmp(other))
+ }
+}
+
+impl Ord for Mod3 {
+ fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+ (self.0 % 3).cmp(&(other.0 % 3))
+ }
+}
+
#[test]
fn test_iterator_max() {
let v: &[_] = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
assert_eq!(v[..4].iter().cloned().max(), Some(3));
assert_eq!(v.iter().cloned().max(), Some(10));
assert_eq!(v[..0].iter().cloned().max(), None);
+ assert_eq!(v.iter().cloned().map(Mod3).max().map(|x| x.0), Some(8));
}
#[test]
@@ -1096,6 +1123,7 @@ fn test_iterator_min() {
assert_eq!(v[..4].iter().cloned().min(), Some(0));
assert_eq!(v.iter().cloned().min(), Some(0));
assert_eq!(v[..0].iter().cloned().min(), None);
+ assert_eq!(v.iter().cloned().map(Mod3).min().map(|x| x.0), Some(0));
}
#[test]
From 6cc5a3d9cc470e2db2b2a45fcddb2350ac0b039e Mon Sep 17 00:00:00 2001
From: Tim Vermeulen <tvermeulen@me.com>
Date: Tue, 12 Mar 2019 20:24:10 +0100
Subject: [PATCH 5/5] Forward `max` and `min` to `max_by` and `min_by`
respectively
---
src/libcore/iter/traits/iterator.rs | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/libcore/iter/traits/iterator.rs b/src/libcore/iter/traits/iterator.rs
index 980de229fa661..ca7feed0712d1 100644
--- a/src/libcore/iter/traits/iterator.rs
+++ b/src/libcore/iter/traits/iterator.rs
@@ -2008,8 +2008,7 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn max(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
- // switch to y even if it is only equal, to preserve stability.
- select_fold1(self, |x, y| x <= y)
+ self.max_by(Ord::cmp)
}
/// Returns the minimum element of an iterator.
@@ -2034,8 +2033,7 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn min(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
- // only switch to y if it is strictly smaller, to preserve stability.
- select_fold1(self, |x, y| x > y)
+ self.min_by(Ord::cmp)
}
/// Returns the element that gives the maximum value from the