libcore: Ensure min and max functions are consistent for equal inputs

Ryman · Ryman · commit 2a89d695f4ed · 2015-03-30T23:48:26.000+01:00
diff --git a/src/libcore/cmp.rs b/src/libcore/cmp.rs
@@ -362,6 +362,8 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
 
 /// Compare and return the minimum of two values.
 ///
+/// Returns the first argument if the comparison determines them to be equal.
+///
 /// # Examples
 ///
 /// ```
@@ -373,11 +375,13 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
 #[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn min<T: Ord>(v1: T, v2: T) -> T {
-    if v1 < v2 { v1 } else { v2 }
+    if v1 <= v2 { v1 } else { v2 }
 }
 
 /// Compare and return the maximum of two values.
 ///
+/// Returns the second argument if the comparison determines them to be equal.
+///
 /// # Examples
 ///
 /// ```
@@ -389,7 +393,7 @@ pub fn min<T: Ord>(v1: T, v2: T) -> T {
 #[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn max<T: Ord>(v1: T, v2: T) -> T {
-    if v1 > v2 { v1 } else { v2 }
+    if v2 >= v1 { v2 } else { v1 }
 }
 
 /// Compare and return the minimum of two values if there is one.
@@ -427,7 +431,7 @@ pub fn partial_min<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
 
 /// Compare and return the maximum of two values if there is one.
 ///
-/// Returns the first argument if the comparison determines them to be equal.
+/// Returns the second argument if the comparison determines them to be equal.
 ///
 /// # Examples
 ///
@@ -452,8 +456,8 @@ pub fn partial_min<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
 #[unstable(feature = "core")]
 pub fn partial_max<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
     match v1.partial_cmp(&v2) {
-        Some(Less) => Some(v2),
-        Some(Equal) | Some(Greater) => Some(v1),
+        Some(Equal) | Some(Less) => Some(v2),
+        Some(Greater) => Some(v1),
         None => None
     }
 }
diff --git a/src/libcore/iter.rs b/src/libcore/iter.rs
@@ -722,6 +722,9 @@ pub trait Iterator {
 
     /// Consumes the entire iterator to return the maximum element.
     ///
+    /// Returns the rightmost element if the comparison determines two elements
+    /// to be equally maximum.
+    ///
     /// # Examples
     ///
     /// ```
@@ -732,16 +735,19 @@ pub trait Iterator {
     #[stable(feature = "rust1", since = "1.0.0")]
     fn max(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
     {
-        self.fold(None, |max, x| {
+        self.fold(None, |max, y| {
             match max {
-                None    => Some(x),
-                Some(y) => Some(cmp::max(x, y))
+                None    => Some(y),
+                Some(x) => Some(cmp::max(x, y))
             }
         })
     }
 
     /// Consumes the entire iterator to return the minimum element.
     ///
+    /// Returns the leftmost element if the comparison determines two elements
+    /// to be equally minimum.
+    ///
     /// # Examples
     ///
     /// ```
@@ -752,10 +758,10 @@ pub trait Iterator {
     #[stable(feature = "rust1", since = "1.0.0")]
     fn min(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
     {
-        self.fold(None, |min, x| {
+        self.fold(None, |min, y| {
             match min {
-                None    => Some(x),
-                Some(y) => Some(cmp::min(x, y))
+                None    => Some(y),
+                Some(x) => Some(cmp::min(x, y))
             }
         })
     }
@@ -799,7 +805,7 @@ pub trait Iterator {
             Some(x) => {
                 match self.next() {
                     None => return OneElement(x),
-                    Some(y) => if x < y {(x, y)} else {(y,x)}
+                    Some(y) => if x <= y {(x, y)} else {(y, x)}
                 }
             }
         };
@@ -817,19 +823,19 @@ pub trait Iterator {
                 None => {
                     if first < min {
                         min = first;
-                    } else if first > max {
+                    } else if first >= max {
                         max = first;
                     }
                     break;
                 }
                 Some(x) => x
             };
-            if first < second {
-                if first < min {min = first;}
-                if max < second {max = second;}
+            if first <= second {
+                if first < min { min = first }
+                if second >= max { max = second }
             } else {
-                if second < min {min = second;}
-                if max < first {max = first;}
+                if second < min { min = second }
+                if first >= max { max = first }
             }
         }
 
@@ -839,6 +845,9 @@ pub trait Iterator {
     /// Return the element that gives the maximum value from the
     /// specified function.
     ///
+    /// Returns the rightmost element if the comparison determines two elements
+    /// to be equally maximum.
+    ///
     /// # Examples
     ///
     /// ```
@@ -855,14 +864,14 @@ pub trait Iterator {
         Self: Sized,
         F: FnMut(&Self::Item) -> B,
     {
-        self.fold(None, |max: Option<(Self::Item, B)>, x| {
-            let x_val = f(&x);
+        self.fold(None, |max: Option<(Self::Item, B)>, y| {
+            let y_val = f(&y);
             match max {
-                None             => Some((x, x_val)),
-                Some((y, y_val)) => if x_val > y_val {
-                    Some((x, x_val))
-                } else {
+                None             => Some((y, y_val)),
+                Some((x, x_val)) => if y_val >= x_val {
                     Some((y, y_val))
+                } else {
+                    Some((x, x_val))
                 }
             }
         }).map(|(x, _)| x)
@@ -871,6 +880,9 @@ pub trait Iterator {
     /// Return the element that gives the minimum value from the
     /// specified function.
     ///
+    /// Returns the leftmost element if the comparison determines two elements
+    /// to be equally minimum.
+    ///
     /// # Examples
     ///
     /// ```
@@ -887,11 +899,11 @@ pub trait Iterator {
         Self: Sized,
         F: FnMut(&Self::Item) -> B,
     {
-        self.fold(None, |min: Option<(Self::Item, B)>, x| {
-            let x_val = f(&x);
+        self.fold(None, |min: Option<(Self::Item, B)>, y| {
+            let y_val = f(&y);
             match min {
-                None             => Some((x, x_val)),
-                Some((y, y_val)) => if x_val < y_val {
+                None             => Some((y, y_val)),
+                Some((x, x_val)) => if x_val <= y_val {
                     Some((x, x_val))
                 } else {
                     Some((y, y_val))
diff --git a/src/test/run-pass/minmax-stability-issue-23687.rs b/src/test/run-pass/minmax-stability-issue-23687.rs
@@ -0,0 +1,83 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(core)]
+use std::fmt::Debug;
+use std::cmp::{self, PartialOrd, Ordering};
+use std::iter::MinMaxResult::MinMax;
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+struct Foo {
+    n: u8,
+    name: &'static str
+}
+
+impl PartialOrd for Foo {
+    fn partial_cmp(&self, other: &Foo) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for Foo {
+    fn cmp(&self, other: &Foo) -> Ordering {
+        self.n.cmp(&other.n)
+    }
+}
+
+fn main() {
+    let a = Foo { n: 4, name: "a" };
+    let b = Foo { n: 4, name: "b" };
+    let c = Foo { n: 8, name: "c" };
+    let d = Foo { n: 8, name: "d" };
+    let e = Foo { n: 22, name: "e" };
+    let f = Foo { n: 22, name: "f" };
+
+    let data = [a, b, c, d, e, f];
+
+    // `min` should return the left when the values are equal
+    assert_eq!(data.iter().min(), Some(&a));
+    assert_eq!(data.iter().min_by(|a| a.n), Some(&a));
+    assert_eq!(cmp::min(a, b), a);
+    assert_eq!(cmp::min(b, a), b);
+    assert_eq!(cmp::partial_min(a, b), Some(a));
+    assert_eq!(cmp::partial_min(b, a), Some(b));
+
+    // `max` should return the right when the values are equal
+    assert_eq!(data.iter().max(), Some(&f));
+    assert_eq!(data.iter().max_by(|a| a.n), Some(&f));
+    assert_eq!(cmp::max(e, f), f);
+    assert_eq!(cmp::max(f, e), e);
+    assert_eq!(cmp::partial_max(e, f), Some(f));
+    assert_eq!(cmp::partial_max(f, e), Some(e));
+
+    // Similar for `min_max`
+    assert_eq!(data.iter().min_max(), MinMax(&a, &f));
+    assert_eq!(data[1..5].iter().min_max(), MinMax(&b, &e));
+    assert_eq!(data[2..4].iter().min_max(), MinMax(&c, &d));
+
+    let mut presorted = data.to_vec();
+    presorted.sort();
+    assert_stable(&presorted);
+
+    let mut presorted = data.to_vec();
+    presorted.sort_by(|a, b| a.cmp(b));
+    assert_stable(&presorted);
+
+    // Assert that sorted and min/max are the same
+    fn assert_stable<T: Ord + Debug>(presorted: &[T]) {
+        for slice in presorted.windows(2) {
+            let a = &slice[0];
+            let b = &slice[1];
+
+            assert_eq!(a, cmp::min(a, b));
+            assert_eq!(b, cmp::max(a, b));
+        }
+    }
+}
