Rollup of 5 pull requests

library/alloc/src/vec/mod.rs

@@ -619,7 +619,7 @@ impl<T> Vec<T> {
     /// use std::alloc::{alloc, Layout};
     ///
     /// fn main() {
-    ///     let layout = Layout::array::<u32>(16).expect("overflow cannot happen");
+    ///     let layout = Layout::array::<u32>(16).expect("16 u32s take 64 bytes, so it shouldn't overflow");
     ///
     ///     let vec = unsafe {
     ///         let mem = alloc(layout).cast::<u32>();
@@ -719,7 +719,7 @@ impl<T> Vec<T> {
     /// use std::ptr::NonNull;
     ///
     /// fn main() {
-    ///     let layout = Layout::array::<u32>(16).expect("overflow cannot happen");
+    ///     let layout = Layout::array::<u32>(16).expect("16 u32s take 64 bytes, so it shouldn't overflow");
     ///
     ///     let vec = unsafe {
     ///         let Some(mem) = NonNull::new(alloc(layout).cast::<u32>()) else {
@@ -1162,7 +1162,7 @@ impl<T, A: Allocator> Vec<T, A> {
     /// use std::alloc::{AllocError, Allocator, Global, Layout};
     ///
     /// fn main() {
-    ///     let layout = Layout::array::<u32>(16).expect("overflow cannot happen");
+    ///     let layout = Layout::array::<u32>(16).expect("16 u32s take 64 bytes, so it shouldn't overflow");
     ///
     ///     let vec = unsafe {
     ///         let mem = match Global.allocate(layout) {
@@ -1277,7 +1277,7 @@ impl<T, A: Allocator> Vec<T, A> {
     /// use std::alloc::{AllocError, Allocator, Global, Layout};
     ///
     /// fn main() {
-    ///     let layout = Layout::array::<u32>(16).expect("overflow cannot happen");
+    ///     let layout = Layout::array::<u32>(16).expect("16 u32s take 64 bytes, so it shouldn't overflow");
     ///
     ///     let vec = unsafe {
     ///         let mem = match Global.allocate(layout) {
@@ -1521,7 +1521,7 @@ impl<T, A: Allocator> Vec<T, A> {
     ///
     ///     Ok(output)
     /// }
-    /// # process_data(&[1, 2, 3]).expect("why is the test harness OOMing on 12 bytes?");
+    /// # process_data(&[1, 2, 3]).expect("this test needs 12 bytes, so it shouldn't fail");
     /// ```
     #[stable(feature = "try_reserve", since = "1.57.0")]
     pub fn try_reserve(&mut self, additional: usize) -> Result<(), TryReserveError> {
@@ -1564,7 +1564,7 @@ impl<T, A: Allocator> Vec<T, A> {
     ///
     ///     Ok(output)
     /// }
-    /// # process_data(&[1, 2, 3]).expect("why is the test harness OOMing on 12 bytes?");
+    /// # process_data(&[1, 2, 3]).expect("this test needs 12 bytes, so it shouldn't fail");
     /// ```
     #[stable(feature = "try_reserve", since = "1.57.0")]
     pub fn try_reserve_exact(&mut self, additional: usize) -> Result<(), TryReserveError> {
@@ -1648,7 +1648,7 @@ impl<T, A: Allocator> Vec<T, A> {
     /// let mut vec = Vec::with_capacity(10);
     /// vec.extend([1, 2, 3]);
     /// assert!(vec.capacity() >= 10);
-    /// vec.try_shrink_to_fit().expect("why is the test harness failing to shrink to 12 bytes");
+    /// vec.try_shrink_to_fit().expect("for this test, shrink shouldn't fail");
     /// assert!(vec.capacity() >= 3);
     /// ```
     #[unstable(feature = "vec_fallible_shrink", issue = "152350")]
@@ -1678,7 +1678,7 @@ impl<T, A: Allocator> Vec<T, A> {
     /// let mut vec = Vec::with_capacity(10);
     /// vec.extend([1, 2, 3]);
     /// assert!(vec.capacity() >= 10);
-    /// vec.try_shrink_to(4).expect("why is the test harness failing to shrink to 12 bytes");
+    /// vec.try_shrink_to(4).expect("for this test, shrink shouldn't fail");
     /// assert!(vec.capacity() >= 4);
     /// vec.try_shrink_to(0).expect("this is a no-op and thus the allocator isn't involved.");
     /// assert!(vec.capacity() >= 3);
@@ -3688,7 +3688,10 @@ impl<T, A: Allocator, const N: usize> Vec<[T; N], A> {
     pub fn into_flattened(self) -> Vec<T, A> {
         let (ptr, len, cap, alloc) = self.into_raw_parts_with_alloc();
         let (new_len, new_cap) = if T::IS_ZST {
-            (len.checked_mul(N).expect("vec len overflow"), usize::MAX)
+            (
+                len.checked_mul(N).expect("the product of vec len and N shouldn't overflow"),
+                usize::MAX,
+            )
         } else {
             // SAFETY:
             // - `cap * N` cannot overflow because the allocation is already in

library/alloctests/tests/vec.rs

@@ -2558,7 +2558,7 @@ fn test_extend_from_within_panicking_clone() {
 }
 
 #[test]
-#[should_panic = "vec len overflow"]
+#[should_panic = "the product of vec len and N shouldn't overflow"]
 fn test_into_flattened_size_overflow() {
     let v = vec![[(); usize::MAX]; 2];
     let _ = v.into_flattened();

library/core/src/array/mod.rs

@@ -410,23 +410,41 @@ where
 const impl<T: [const] PartialOrd, const N: usize> PartialOrd for [T; N] {
     #[inline]
     fn partial_cmp(&self, other: &[T; N]) -> Option<Ordering> {
-        PartialOrd::partial_cmp(&&self[..], &&other[..])
+        <[T] as PartialOrd>::partial_cmp(self, other)
     }
+
     #[inline]
     fn lt(&self, other: &[T; N]) -> bool {
-        PartialOrd::lt(&&self[..], &&other[..])
+        <[T] as PartialOrd>::lt(self, other)
     }
     #[inline]
     fn le(&self, other: &[T; N]) -> bool {
-        PartialOrd::le(&&self[..], &&other[..])
+        <[T] as PartialOrd>::le(self, other)
     }
     #[inline]
     fn ge(&self, other: &[T; N]) -> bool {
-        PartialOrd::ge(&&self[..], &&other[..])
+        <[T] as PartialOrd>::ge(self, other)
     }
     #[inline]
     fn gt(&self, other: &[T; N]) -> bool {
-        PartialOrd::gt(&&self[..], &&other[..])
+        <[T] as PartialOrd>::gt(self, other)
+    }
+
+    #[inline]
+    fn __chaining_lt(&self, other: &[T; N]) -> ControlFlow<bool> {
+        <[T] as PartialOrd>::__chaining_lt(self, other)
+    }
+    #[inline]
+    fn __chaining_le(&self, other: &[T; N]) -> ControlFlow<bool> {
+        <[T] as PartialOrd>::__chaining_le(self, other)
+    }
+    #[inline]
+    fn __chaining_ge(&self, other: &[T; N]) -> ControlFlow<bool> {
+        <[T] as PartialOrd>::__chaining_ge(self, other)
+    }
+    #[inline]
+    fn __chaining_gt(&self, other: &[T; N]) -> ControlFlow<bool> {
+        <[T] as PartialOrd>::__chaining_gt(self, other)
     }
 }
 

library/core/src/cmp.rs

@@ -743,6 +743,54 @@ impl<T: Clone> Clone for Reverse<T> {
     }
 }
 
+/// A pair where ordering and equality work on only the `key`, ignoring the `value`.
+///
+/// Used to implement `Iterator::min_by_key` as `map`+`min`, for example.
+#[derive(Debug, Copy, Clone)]
+pub(crate) struct KeyAndValue<K, V> {
+    pub key: K,
+    pub value: V,
+}
+impl<K: PartialEq, V> PartialEq for KeyAndValue<K, V> {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        self.key == other.key
+    }
+    #[inline]
+    fn ne(&self, other: &Self) -> bool {
+        self.key != other.key
+    }
+}
+impl<K: Eq, V> Eq for KeyAndValue<K, V> {}
+impl<K: PartialOrd, V> PartialOrd for KeyAndValue<K, V> {
+    #[inline]
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        PartialOrd::partial_cmp(&self.key, &other.key)
+    }
+    #[inline]
+    fn lt(&self, other: &Self) -> bool {
+        self.key < other.key
+    }
+    #[inline]
+    fn le(&self, other: &Self) -> bool {
+        self.key <= other.key
+    }
+    #[inline]
+    fn gt(&self, other: &Self) -> bool {
+        self.key > other.key
+    }
+    #[inline]
+    fn ge(&self, other: &Self) -> bool {
+        self.key >= other.key
+    }
+}
+impl<K: Ord, V> Ord for KeyAndValue<K, V> {
+    #[inline]
+    fn cmp(&self, other: &Self) -> Ordering {
+        Ord::cmp(&self.key, &other.key)
+    }
+}
+
 /// Trait for types that form a [total order](https://en.wikipedia.org/wiki/Total_order).
 ///
 /// Implementations must be consistent with the [`PartialOrd`] implementation, and ensure `max`,

library/core/src/iter/traits/iterator.rs

@@ -6,7 +6,7 @@ use super::super::{
 };
 use super::TrustedLen;
 use crate::array;
-use crate::cmp::{self, Ordering};
+use crate::cmp::{self, KeyAndValue, Ordering};
 use crate::marker::Destruct;
 use crate::num::NonZero;
 use crate::ops::{ChangeOutputType, ControlFlow, FromResidual, Residual, Try};
@@ -3248,7 +3248,7 @@ pub const trait Iterator {
         Self: Sized,
         Self::Item: Ord,
     {
-        self.max_by(Ord::cmp)
+        self.reduce(Ord::max)
     }
 
     /// Returns the minimum element of an iterator.
@@ -3285,7 +3285,7 @@ pub const trait Iterator {
         Self: Sized,
         Self::Item: Ord,
     {
-        self.min_by(Ord::cmp)
+        self.reduce(Ord::min)
     }
 
     /// Returns the element that gives the maximum value from the
@@ -3308,18 +3308,17 @@ pub const trait Iterator {
         Self: Sized,
         F: FnMut(&Self::Item) -> B,
     {
-        #[inline]
-        fn key<T, B>(mut f: impl FnMut(&T) -> B) -> impl FnMut(T) -> (B, T) {
-            move |x| (f(&x), x)
-        }
+        // If we implemented this via `max_by` that would force it to use `B::cmp`.
+        // By using `max` over `KeyAndValue`, it instead ends up calling `B::lt`
+        // (via `KeyAndValue::max`), which is often overridden more efficiently.
 
         #[inline]
-        fn compare<T, B: Ord>((x_p, _): &(B, T), (y_p, _): &(B, T)) -> Ordering {
-            x_p.cmp(y_p)
+        fn key<T, B>(mut f: impl FnMut(&T) -> B) -> impl FnMut(T) -> KeyAndValue<B, T> {
+            move |value| KeyAndValue { key: f(&value), value }
         }
 
-        let (_, x) = self.map(key(f)).max_by(compare)?;
-        Some(x)
+        let KeyAndValue { value, .. } = self.map(key(f)).max()?;
+        Some(value)
     }
 
     /// Returns the element that gives the maximum value with respect to the
@@ -3370,18 +3369,17 @@ pub const trait Iterator {
         Self: Sized,
         F: FnMut(&Self::Item) -> B,
     {
-        #[inline]
-        fn key<T, B>(mut f: impl FnMut(&T) -> B) -> impl FnMut(T) -> (B, T) {
-            move |x| (f(&x), x)
-        }
+        // If we implemented this via `min_by` that would force it to use `B::cmp`.
+        // By using `min` over `KeyAndValue`, it instead ends up calling `B::lt`
+        // (via `KeyAndValue::min`), which is often overridden more efficiently.
 
         #[inline]
-        fn compare<T, B: Ord>((x_p, _): &(B, T), (y_p, _): &(B, T)) -> Ordering {
-            x_p.cmp(y_p)
+        fn key<T, B>(mut f: impl FnMut(&T) -> B) -> impl FnMut(T) -> KeyAndValue<B, T> {
+            move |value| KeyAndValue { key: f(&value), value }
         }
 
-        let (_, x) = self.map(key(f)).min_by(compare)?;
-        Some(x)
+        let KeyAndValue { value, .. } = self.map(key(f)).min()?;
+        Some(value)
     }
 
     /// Returns the element that gives the minimum value with respect to the

library/coretests/tests/iter/traits/iterator.rs

@@ -1,4 +1,5 @@
 use core::cell::RefCell;
+use core::cmp::Ordering;
 use core::iter::zip;
 use core::num::NonZero;
 
@@ -17,13 +18,13 @@ impl PartialEq for Mod3 {
 impl Eq for Mod3 {}
 
 impl PartialOrd for Mod3 {
-    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))
     }
 }
 
 impl Ord for Mod3 {
-    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+    fn cmp(&self, other: &Self) -> Ordering {
         (self.0 % 3).cmp(&(other.0 % 3))
     }
 }
@@ -75,8 +76,6 @@ fn test_lt() {
 
 #[test]
 fn test_cmp_by() {
-    use core::cmp::Ordering;
-
     let f = |x: i32, y: i32| (x * x).cmp(&y);
     let xs = || [1, 2, 3, 4].iter().copied();
     let ys = || [1, 4, 16].iter().copied();
@@ -91,8 +90,6 @@ fn test_cmp_by() {
 
 #[test]
 fn test_partial_cmp_by() {
-    use core::cmp::Ordering;
-
     let f = |x: i32, y: i32| (x * x).partial_cmp(&y);
     let xs = || [1, 2, 3, 4].iter().copied();
     let ys = || [1, 4, 16].iter().copied();
@@ -722,3 +719,62 @@ fn _empty_impl_all_auto_traits<T>() {
 
     all_auto_traits::<std::iter::Empty<T>>();
 }
+
+#[test]
+fn test_iterator_min_max_use_ord_min_max() {
+    // There's no stable guarantee that the iterator methods use these, but they were added
+    // on `Ord` (as opposed to just the functions in `cmp`) so that they could be overridden
+    // when a more efficient implementation is available, so we should probably use them.
+
+    let a = [OnlyMinMax(3), OnlyMinMax(1), OnlyMinMax(5), OnlyMinMax(9), OnlyMinMax(7)];
+    assert_eq!(a.iter().copied().min(), Some(OnlyMinMax(1)));
+    assert_eq!(a.iter().copied().max(), Some(OnlyMinMax(9)));
+
+    #[derive(Debug, Copy, Clone, Eq, PartialEq)]
+    struct OnlyMinMax(i32);
+    impl PartialOrd for OnlyMinMax {
+        fn partial_cmp(&self, _other: &Self) -> Option<Ordering> {
+            unimplemented!()
+        }
+    }
+    impl Ord for OnlyMinMax {
+        fn cmp(&self, _other: &Self) -> Ordering {
+            unimplemented!()
+        }
+        fn min(self, other: Self) -> Self {
+            Self(Ord::min(self.0, other.0))
+        }
+        fn max(self, other: Self) -> Self {
+            Self(Ord::max(self.0, other.0))
+        }
+    }
+}
+
+#[test]
+fn test_iterator_min_max_by_key_use_lt() {
+    // The exact method is certainly not a stable guarantee. The important part
+    // is that they use a simple `-> bool` method as opposed to three-way `cmp`.
+    // If they used `gt` instead, or something, that wouldn't be the end of the world,
+    // but `lt` tends to be best optimized because that's the one that C++ has
+    // traditionally used in standard library templates.
+
+    let a = [3, 1, 5, 9, 7];
+    assert_eq!(a.iter().copied().min_by_key(|x| OnlyLt(*x)), Some(1));
+    assert_eq!(a.iter().copied().max_by_key(|x| OnlyLt(*x)), Some(9));
+
+    #[derive(Debug, Copy, Clone, Eq, PartialEq)]
+    struct OnlyLt(i32);
+    impl PartialOrd for OnlyLt {
+        fn partial_cmp(&self, _other: &Self) -> Option<Ordering> {
+            unimplemented!()
+        }
+        fn lt(&self, other: &Self) -> bool {
+            self.0 < other.0
+        }
+    }
+    impl Ord for OnlyLt {
+        fn cmp(&self, _other: &Self) -> Ordering {
+            unimplemented!()
+        }
+    }
+}