about summary refs log tree commit diff
diff options
context:
space:
mode:
authorLukas Bergdoll <lukas.bergdoll@gmail.com>2024-07-31 11:28:11 +0200
committerLukas Bergdoll <lukas.bergdoll@gmail.com>2024-07-31 11:36:57 +0200
commitafc404fdfc1559b4ba2846a054347e806bb8f465 (patch)
treef95eb1b707d1c2719bf480a2928a6dc11d124982
parent00ce238885825e5e987dbcd4fef266e96157303d (diff)
downloadrust-afc404fdfc1559b4ba2846a054347e806bb8f465.tar.gz
rust-afc404fdfc1559b4ba2846a054347e806bb8f465.zip
Apply review comments
- Use if the implementation of [`Ord`] for `T`
  language
- Link to total order wiki page
- Rework total order help and examples
- Improve language to be more precise and less
  prone to misunderstandings.
- Fix usage of `sort_unstable_by` in `sort_by`
  example
- Fix missing author mention
- Use more consistent example input for sort
- Use more idiomatic assert_eq! in examples
- Use more natural "comparison function" language
  instead of "comparator function"
-rw-r--r--library/alloc/src/slice.rs88
-rw-r--r--library/core/src/slice/mod.rs80
-rw-r--r--library/core/src/slice/sort/shared/smallsort.rs13
-rw-r--r--library/core/src/slice/sort/unstable/mod.rs2
4 files changed, 97 insertions, 86 deletions
diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs
index 9e222c6072f..21270dbed0c 100644
--- a/library/alloc/src/slice.rs
+++ b/library/alloc/src/slice.rs
@@ -179,15 +179,25 @@ impl<T> [T] {
     /// This sort is stable (i.e., does not reorder equal elements) and *O*(*n* \* log(*n*))
     /// worst-case.
     ///
-    /// If `T: Ord` does not implement a total order the resulting order is unspecified. All
-    /// original elements will remain in the slice and any possible modifications via interior
-    /// mutability are observed in the input. Same is true if `T: Ord` panics.
+    /// If the implementation of [`Ord`] for `T` does not implement a [total order] the resulting
+    /// order of elements in the slice is unspecified. All original elements will remain in the
+    /// slice and any possible modifications via interior mutability are observed in the input. Same
+    /// is true if the implementation of [`Ord`] for `T` panics.
     ///
     /// When applicable, unstable sorting is preferred because it is generally faster than stable
     /// sorting and it doesn't allocate auxiliary memory. See
     /// [`sort_unstable`](slice::sort_unstable). The exception are partially sorted slices, which
     /// may be better served with `slice::sort`.
     ///
+    /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] requires
+    /// additional precautions. For example Rust defines `NaN != NaN`, which doesn't fulfill the
+    /// reflexivity requirement posed by [`Ord`]. By using an alternative comparison function with
+    /// [`slice::sort_by`] such as [`f32::total_cmp`] or [`f64::total_cmp`] that defines a [total
+    /// order] users can sort slices containing floating point numbers. Alternatively, if one can
+    /// guarantee that all values in the slice are comparable with [`PartialOrd::partial_cmp`] *and*
+    /// the implementation forms a [total order], it's possible to sort the slice with `sort_by(|a,
+    /// b| a.partial_cmp(b).unwrap())`.
+    ///
     /// # Current implementation
     ///
     /// The current implementation is based on [driftsort] by Orson Peters and Lukas Bergdoll, which
@@ -201,18 +211,19 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `T: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `T` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [-5, 4, 1, -3, 2];
+    /// let mut v = [4, -5, 1, -3, 2];
     ///
     /// v.sort();
-    /// assert!(v == [-5, -3, 1, 2, 4]);
+    /// assert_eq!(v, [-5, -3, 1, 2, 4]);
     /// ```
     ///
     /// [driftsort]: https://github.com/Voultapher/driftsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[cfg(not(no_global_oom_handling))]
     #[rustc_allow_incoherent_impl]
     #[stable(feature = "rust1", since = "1.0.0")]
@@ -224,30 +235,19 @@ impl<T> [T] {
         stable_sort(self, T::lt);
     }
 
-    /// Sorts the slice with a comparator function, preserving initial order of equal elements.
+    /// Sorts the slice with a comparison function, preserving initial order of equal elements.
     ///
     /// This sort is stable (i.e., does not reorder equal elements) and *O*(*n* \* log(*n*))
     /// worst-case.
     ///
-    /// The comparator function should define a total ordering for the elements in the slice. If the
-    /// ordering is not total, the order of the elements is unspecified.
-    ///
-    /// If the comparator function does not implement a total order the resulting order is
-    /// unspecified. All original elements will remain in the slice and any possible modifications
-    /// via interior mutability are observed in the input. Same is true if the comparator function
-    /// panics. A total order (for all `a`, `b` and `c`):
-    ///
-    /// * total and antisymmetric: exactly one of `a < b`, `a == b` or `a > b` is true, and
-    /// * transitive, `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
+    /// If the comparison function `compare` does not implement a [total order] the resulting order
+    /// of elements in the slice is unspecified. All original elements will remain in the slice and
+    /// any possible modifications via interior mutability are observed in the input. Same is true
+    /// if `compare` panics.
     ///
-    /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use
-    /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`.
-    ///
-    /// ```
-    /// let mut floats = [5f64, 4.0, 1.0, 3.0, 2.0];
-    /// floats.sort_unstable_by(|a, b| a.partial_cmp(b).unwrap());
-    /// assert_eq!(floats, [1.0, 2.0, 3.0, 4.0, 5.0]);
-    /// ```
+    /// For example `|a, b| (a - b).cmp(a)` is a comparison function that is neither transitive nor
+    /// reflexive nor total, `a < b < c < a` with `a = 1, b = 2, c = 3`. For more information and
+    /// examples see the [`Ord`] documentation.
     ///
     /// # Current implementation
     ///
@@ -262,21 +262,22 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `compare` does not implement a total order.
+    /// May panic if `compare` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [5, 4, 1, 3, 2];
+    /// let mut v = [4, -5, 1, -3, 2];
     /// v.sort_by(|a, b| a.cmp(b));
-    /// assert!(v == [1, 2, 3, 4, 5]);
+    /// assert_eq!(v, [-5, -3, 1, 2, 4]);
     ///
     /// // reverse sorting
     /// v.sort_by(|a, b| b.cmp(a));
-    /// assert!(v == [5, 4, 3, 2, 1]);
+    /// assert_eq!(v, [4, 2, 1, -3, -5]);
     /// ```
     ///
     /// [driftsort]: https://github.com/Voultapher/driftsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[cfg(not(no_global_oom_handling))]
     #[rustc_allow_incoherent_impl]
     #[stable(feature = "rust1", since = "1.0.0")]
@@ -293,9 +294,10 @@ impl<T> [T] {
     /// This sort is stable (i.e., does not reorder equal elements) and *O*(*m* \* *n* \* log(*n*))
     /// worst-case, where the key function is *O*(*m*).
     ///
-    /// If `K: Ord` does not implement a total order the resulting order is unspecified.
-    /// All original elements will remain in the slice and any possible modifications via interior
-    /// mutability are observed in the input. Same is true if `K: Ord` panics.
+    /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting
+    /// order of elements in the slice is unspecified. All original elements will remain in the
+    /// slice and any possible modifications via interior mutability are observed in the input. Same
+    /// is true if the implementation of [`Ord`] for `K` panics.
     ///
     /// # Current implementation
     ///
@@ -310,18 +312,19 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `K: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `K` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [-5i32, 4, 1, -3, 2];
+    /// let mut v = [4i32, -5, 1, -3, 2];
     ///
     /// v.sort_by_key(|k| k.abs());
-    /// assert!(v == [1, 2, -3, 4, -5]);
+    /// assert_eq!(v, [1, 2, -3, 4, -5]);
     /// ```
     ///
     /// [driftsort]: https://github.com/Voultapher/driftsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[cfg(not(no_global_oom_handling))]
     #[rustc_allow_incoherent_impl]
     #[stable(feature = "slice_sort_by_key", since = "1.7.0")]
@@ -343,9 +346,10 @@ impl<T> [T] {
     /// storage to remember the results of key evaluation. The order of calls to the key function is
     /// unspecified and may change in future versions of the standard library.
     ///
-    /// If `K: Ord` does not implement a total order the resulting order is unspecified.
-    /// All original elements will remain in the slice and any possible modifications via interior
-    /// mutability are observed in the input. Same is true if `K: Ord` panics.
+    /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting
+    /// order of elements in the slice is unspecified. All original elements will remain in the
+    /// slice and any possible modifications via interior mutability are observed in the input. Same
+    /// is true if the implementation of [`Ord`] for `K` panics.
     ///
     /// For simple key functions (e.g., functions that are property accesses or basic operations),
     /// [`sort_by_key`](slice::sort_by_key) is likely to be faster.
@@ -364,18 +368,20 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `K: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `K` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [-5i32, 4, 32, -3, 2];
+    /// let mut v = [4i32, -5, 1, -3, 2, 10];
     ///
+    /// // Strings are sorted by lexicographical order.
     /// v.sort_by_cached_key(|k| k.to_string());
-    /// assert!(v == [-3, -5, 2, 32, 4]);
+    /// assert_eq!(v, [-3, -5, 1, 10, 2, 4]);
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[cfg(not(no_global_oom_handling))]
     #[rustc_allow_incoherent_impl]
     #[stable(feature = "slice_sort_by_cached_key", since = "1.34.0")]
diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs
index 8916d43a8f2..223ba96c475 100644
--- a/library/core/src/slice/mod.rs
+++ b/library/core/src/slice/mod.rs
@@ -2884,9 +2884,19 @@ impl<T> [T] {
     /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not
     /// allocate), and *O*(*n* \* log(*n*)) worst-case.
     ///
-    /// If `T: Ord` does not implement a total order the resulting order is unspecified. All
-    /// original elements will remain in the slice and any possible modifications via interior
-    /// mutability are observed in the input. Same is true if `T: Ord` panics.
+    /// If the implementation of [`Ord`] for `T` does not implement a [total order] the resulting
+    /// order of elements in the slice is unspecified. All original elements will remain in the
+    /// slice and any possible modifications via interior mutability are observed in the input. Same
+    /// is true if the implementation of [`Ord`] for `T` panics.
+    ///
+    /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] requires
+    /// additional precautions. For example Rust defines `NaN != NaN`, which doesn't fulfill the
+    /// reflexivity requirement posed by [`Ord`]. By using an alternative comparison function with
+    /// [`slice::sort_unstable_by`] such as [`f32::total_cmp`] or [`f64::total_cmp`] that defines a
+    /// [total order] users can sort slices containing floating point numbers. Alternatively, if one
+    /// can guarantee that all values in the slice are comparable with [`PartialOrd::partial_cmp`]
+    /// *and* the implementation forms a [total order], it's possible to sort the slice with
+    /// `sort_unstable_by(|a, b| a.partial_cmp(b).unwrap())`.
     ///
     /// # Current implementation
     ///
@@ -2900,18 +2910,19 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `T: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `T` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [-5, 4, 1, -3, 2];
+    /// let mut v = [4, -5, 1, -3, 2];
     ///
     /// v.sort_unstable();
-    /// assert!(v == [-5, -3, 1, 2, 4]);
+    /// assert_eq!(v, [-5, -3, 1, 2, 4]);
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "sort_unstable", since = "1.20.0")]
     #[inline]
     pub fn sort_unstable(&mut self)
@@ -2921,31 +2932,20 @@ impl<T> [T] {
         sort::unstable::sort(self, &mut T::lt);
     }
 
-    /// Sorts the slice with a comparator function, **without** preserving the initial order of
+    /// Sorts the slice with a comparison function, **without** preserving the initial order of
     /// equal elements.
     ///
     /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not
     /// allocate), and *O*(*n* \* log(*n*)) worst-case.
     ///
-    /// The comparator function should define a total ordering for the elements in the slice. If the
-    /// ordering is not total, the order of the elements is unspecified.
+    /// If the comparison function `compare` does not implement a [total order] the resulting order
+    /// of elements in the slice is unspecified. All original elements will remain in the slice and
+    /// any possible modifications via interior mutability are observed in the input. Same is true
+    /// if `compare` panics.
     ///
-    /// If the comparator function does not implement a total order the resulting order is
-    /// unspecified. All original elements will remain in the slice and any possible modifications
-    /// via interior mutability are observed in the input. Same is true if the comparator function
-    /// panics. A total order (for all `a`, `b` and `c`):
-    ///
-    /// * total and antisymmetric: exactly one of `a < b`, `a == b` or `a > b` is true, and
-    /// * transitive, `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
-    ///
-    /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use
-    /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`.
-    ///
-    /// ```
-    /// let mut floats = [5f64, 4.0, 1.0, 3.0, 2.0];
-    /// floats.sort_unstable_by(|a, b| a.partial_cmp(b).unwrap());
-    /// assert_eq!(floats, [1.0, 2.0, 3.0, 4.0, 5.0]);
-    /// ```
+    /// For example `|a, b| (a - b).cmp(a)` is a comparison function that is neither transitive nor
+    /// reflexive nor total, `a < b < c < a` with `a = 1, b = 2, c = 3`. For more information and
+    /// examples see the [`Ord`] documentation.
     ///
     /// # Current implementation
     ///
@@ -2959,21 +2959,22 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `compare` does not implement a total order.
+    /// May panic if `compare` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [5, 4, 1, 3, 2];
+    /// let mut v = [4, -5, 1, -3, 2];
     /// v.sort_unstable_by(|a, b| a.cmp(b));
-    /// assert!(v == [1, 2, 3, 4, 5]);
+    /// assert_eq!(v, [-5, -3, 1, 2, 4]);
     ///
     /// // reverse sorting
     /// v.sort_unstable_by(|a, b| b.cmp(a));
-    /// assert!(v == [5, 4, 3, 2, 1]);
+    /// assert_eq!(v, [4, 2, 1, -3, -5]);
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "sort_unstable", since = "1.20.0")]
     #[inline]
     pub fn sort_unstable_by<F>(&mut self, mut compare: F)
@@ -2989,9 +2990,10 @@ impl<T> [T] {
     /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not
     /// allocate), and *O*(*n* \* log(*n*)) worst-case.
     ///
-    /// If `K: Ord` does not implement a total order the resulting order is unspecified.
-    /// All original elements will remain in the slice and any possible modifications via interior
-    /// mutability are observed in the input. Same is true if `K: Ord` panics.
+    /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting
+    /// order of elements in the slice is unspecified. All original elements will remain in the
+    /// slice and any possible modifications via interior mutability are observed in the input. Same
+    /// is true if the implementation of [`Ord`] for `K` panics.
     ///
     /// # Current implementation
     ///
@@ -3005,18 +3007,19 @@ impl<T> [T] {
     ///
     /// # Panics
     ///
-    /// May panic if `K: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `K` does not implement a [total order].
     ///
     /// # Examples
     ///
     /// ```
-    /// let mut v = [-5i32, 4, 1, -3, 2];
+    /// let mut v = [4i32, -5, 1, -3, 2];
     ///
     /// v.sort_unstable_by_key(|k| k.abs());
-    /// assert!(v == [1, 2, -3, 4, -5]);
+    /// assert_eq!(v, [1, 2, -3, 4, -5]);
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "sort_unstable", since = "1.20.0")]
     #[inline]
     pub fn sort_unstable_by_key<K, F>(&mut self, mut f: F)
@@ -3054,7 +3057,7 @@ impl<T> [T] {
     ///
     /// Panics when `index >= len()`, meaning it always panics on empty slices.
     ///
-    /// May panic if `T: Ord` does not implement a total order.
+    /// May panic if the implementation of [`Ord`] for `T` does not implement a [total order].
     ///
     /// # Examples
     ///
@@ -3078,6 +3081,7 @@ impl<T> [T] {
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")]
     #[inline]
     pub fn select_nth_unstable(&mut self, index: usize) -> (&mut [T], &mut T, &mut [T])
@@ -3114,7 +3118,7 @@ impl<T> [T] {
     ///
     /// Panics when `index >= len()`, meaning it always panics on empty slices.
     ///
-    /// May panic if `compare` does not implement a total order.
+    /// May panic if `compare` does not implement a [total order].
     ///
     /// # Examples
     ///
@@ -3138,6 +3142,7 @@ impl<T> [T] {
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")]
     #[inline]
     pub fn select_nth_unstable_by<F>(
@@ -3202,6 +3207,7 @@ impl<T> [T] {
     /// ```
     ///
     /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort
+    /// [total order]: https://en.wikipedia.org/wiki/Total_order
     #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")]
     #[inline]
     pub fn select_nth_unstable_by_key<K, F>(
diff --git a/library/core/src/slice/sort/shared/smallsort.rs b/library/core/src/slice/sort/shared/smallsort.rs
index f3b0bf8b018..e168e8a4478 100644
--- a/library/core/src/slice/sort/shared/smallsort.rs
+++ b/library/core/src/slice/sort/shared/smallsort.rs
@@ -835,9 +835,8 @@ unsafe fn bidirectional_merge<T: FreezeMarker, F: FnMut(&T, &T) -> bool>(
         }
 
         // We now should have consumed the full input exactly once. This can only fail if the
-        // user-provided comparison operator fails implements a strict weak ordering as required by
-        // Ord incorrectly, in which case we will panic and never access the inconsistent state in
-        // dst.
+        // user-provided comparison function fails to implement a strict weak ordering. In that case
+        // we panic and never access the inconsistent state in dst.
         if left != left_end || right != right_end {
             panic_on_ord_violation();
         }
@@ -850,11 +849,11 @@ fn panic_on_ord_violation() -> ! {
     // implementation. They are expected to implement a total order as explained in the Ord
     // documentation.
     //
-    // By raising this panic we inform the user, that they have a logic bug in their program. If a
-    // strict weak ordering is not given, the concept of comparison based sorting makes no sense.
-    // E.g.: a < b < c < a
+    // By panicking we inform the user, that they have a logic bug in their program. If a strict
+    // weak ordering is not given, the concept of comparison based sorting cannot yield a sorted
+    // result. E.g.: a < b < c < a
     //
-    // The Ord documentation requires users to implement a total order, arguably that's
+    // The Ord documentation requires users to implement a total order. Arguably that's
     // unnecessarily strict in the context of sorting. Issues only arise if the weaker requirement
     // of a strict weak ordering is violated.
     //
diff --git a/library/core/src/slice/sort/unstable/mod.rs b/library/core/src/slice/sort/unstable/mod.rs
index 692c2d8f7c7..17aa3994bf2 100644
--- a/library/core/src/slice/sort/unstable/mod.rs
+++ b/library/core/src/slice/sort/unstable/mod.rs
@@ -9,7 +9,7 @@ use crate::slice::sort::shared::smallsort::insertion_sort_shift_left;
 pub(crate) mod heapsort;
 pub(crate) mod quicksort;
 
-/// Unstable sort called ipnsort by Lukas Bergdoll.
+/// Unstable sort called ipnsort by Lukas Bergdoll and Orson Peters.
 /// Design document:
 /// <https://github.com/Voultapher/sort-research-rs/blob/main/writeup/ipnsort_introduction/text.md>
 ///