From 644550f3de4805b3dec6ade4a60ecf48c1d2487b Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Sat, 27 Jul 2024 16:47:10 +0200 Subject: [PATCH 1/5] Improve panic message and surrounding documentation for Ord violations The new sort implementations have the ability to detect Ord violations in many cases. This commit improves the message in a way that should help users realize what went wrong in their program. --- .../core/src/slice/sort/shared/smallsort.rs | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/library/core/src/slice/sort/shared/smallsort.rs b/library/core/src/slice/sort/shared/smallsort.rs index 5111ed8756bf1..f3b0bf8b018e6 100644 --- a/library/core/src/slice/sort/shared/smallsort.rs +++ b/library/core/src/slice/sort/shared/smallsort.rs @@ -834,9 +834,10 @@ unsafe fn bidirectional_merge bool>( right = right.add((!left_nonempty) as usize); } - // We now should have consumed the full input exactly once. This can - // only fail if the comparison operator fails to be Ord, in which case - // we will panic and never access the inconsistent state in dst. + // 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. if left != left_end || right != right_end { panic_on_ord_violation(); } @@ -845,7 +846,21 @@ unsafe fn bidirectional_merge bool>( #[inline(never)] fn panic_on_ord_violation() -> ! { - panic!("Ord violation"); + // This is indicative of a logic bug in the user-provided comparison function or Ord + // 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 + // + // 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. + // + // The panic message talks about a total order because that's what the Ord documentation talks + // about and requires, so as to not confuse users. + panic!("user-provided comparison function does not correctly implement a total order"); } #[must_use] From 00ce238885825e5e987dbcd4fef266e96157303d Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Sat, 27 Jul 2024 16:48:42 +0200 Subject: [PATCH 2/5] Improve panic sections for sort*, sort_unstable* and select_nth_unstable* - Move panic information into # Panics section - Fix mentions of T: Ord that should be compare - Add missing information --- library/alloc/src/slice.rs | 16 +++++++++++++--- library/core/src/slice/mod.rs | 27 +++++++++++++++------------ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index c7960b3fb49c3..9e222c6072fd7 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -199,7 +199,9 @@ impl [T] { /// handled without allocation, medium sized slices allocate `self.len()` and beyond that it /// clamps at `self.len() / 2`. /// - /// If `T: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `T: Ord` does not implement a total order. /// /// # Examples /// @@ -258,7 +260,9 @@ impl [T] { /// handled without allocation, medium sized slices allocate `self.len()` and beyond that it /// clamps at `self.len() / 2`. /// - /// If `T: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `compare` does not implement a total order. /// /// # Examples /// @@ -304,7 +308,9 @@ impl [T] { /// handled without allocation, medium sized slices allocate `self.len()` and beyond that it /// clamps at `self.len() / 2`. /// - /// If `K: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `K: Ord` does not implement a total order. /// /// # Examples /// @@ -356,6 +362,10 @@ impl [T] { /// In the worst case, the algorithm allocates temporary storage in a `Vec<(K, usize)>` the /// length of the slice. /// + /// # Panics + /// + /// May panic if `K: Ord` does not implement a total order. + /// /// # Examples /// /// ``` diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 6d3e625bef428..8916d43a8f236 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -2898,7 +2898,9 @@ impl [T] { /// It is typically faster than stable sorting, except in a few special cases, e.g., when the /// slice is partially sorted. /// - /// If `T: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `T: Ord` does not implement a total order. /// /// # Examples /// @@ -2955,7 +2957,9 @@ impl [T] { /// It is typically faster than stable sorting, except in a few special cases, e.g., when the /// slice is partially sorted. /// - /// If `T: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `compare` does not implement a total order. /// /// # Examples /// @@ -2999,7 +3003,9 @@ impl [T] { /// It is typically faster than stable sorting, except in a few special cases, e.g., when the /// slice is partially sorted. /// - /// If `K: Ord` does not implement a total order, the implementation may panic. + /// # Panics + /// + /// May panic if `K: Ord` does not implement a total order. /// /// # Examples /// @@ -3042,15 +3048,14 @@ impl [T] { /// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime /// for all inputs. /// - /// It is typically faster than stable sorting, except in a few special cases, e.g., when the - /// slice is nearly fully sorted, where `slice::sort` may be faster. - /// /// [`sort_unstable`]: slice::sort_unstable /// /// # Panics /// /// Panics when `index >= len()`, meaning it always panics on empty slices. /// + /// May panic if `T: Ord` does not implement a total order. + /// /// # Examples /// /// ``` @@ -3103,15 +3108,14 @@ impl [T] { /// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime /// for all inputs. /// - /// It is typically faster than stable sorting, except in a few special cases, e.g., when the - /// slice is nearly fully sorted, where `slice::sort` may be faster. - /// /// [`sort_unstable`]: slice::sort_unstable /// /// # Panics /// /// Panics when `index >= len()`, meaning it always panics on empty slices. /// + /// May panic if `compare` does not implement a total order. + /// /// # Examples /// /// ``` @@ -3168,15 +3172,14 @@ impl [T] { /// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime /// for all inputs. /// - /// It is typically faster than stable sorting, except in a few special cases, e.g., when the - /// slice is nearly fully sorted, where `slice::sort` may be faster. - /// /// [`sort_unstable`]: slice::sort_unstable /// /// # Panics /// /// Panics when `index >= len()`, meaning it always panics on empty slices. /// + /// May panic if `K: Ord` does not implement a total order. + /// /// # Examples /// /// ``` From afc404fdfc1559b4ba2846a054347e806bb8f465 Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Wed, 31 Jul 2024 11:28:11 +0200 Subject: [PATCH 3/5] 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" --- library/alloc/src/slice.rs | 88 ++++++++++--------- library/core/src/slice/mod.rs | 80 +++++++++-------- .../core/src/slice/sort/shared/smallsort.rs | 13 ++- library/core/src/slice/sort/unstable/mod.rs | 2 +- 4 files changed, 97 insertions(+), 86 deletions(-) diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 9e222c6072fd7..21270dbed0c61 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -179,15 +179,25 @@ impl [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] { /// /// # 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] { 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] { /// /// # 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] { /// 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] { /// /// # 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] { /// 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] { /// /// # 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 8916d43a8f236..223ba96c4755b 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -2884,9 +2884,19 @@ impl [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] { /// /// # 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] { 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] { /// /// # 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(&mut self, mut compare: F) @@ -2989,9 +2990,10 @@ impl [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] { /// /// # 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(&mut self, mut f: F) @@ -3054,7 +3057,7 @@ impl [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] { /// ``` /// /// [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] { /// /// 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] { /// ``` /// /// [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( @@ -3202,6 +3207,7 @@ impl [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( diff --git a/library/core/src/slice/sort/shared/smallsort.rs b/library/core/src/slice/sort/shared/smallsort.rs index f3b0bf8b018e6..e168e8a4478b8 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 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 692c2d8f7c7ba..17aa3994bf20d 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: /// /// From eae7a186b221bb2b4a4bedeb19d5aca658e91c25 Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Thu, 1 Aug 2024 17:37:07 +0200 Subject: [PATCH 4/5] Hide internal sort module --- library/core/src/slice/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 223ba96c4755b..862a18c3f4a4f 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -31,6 +31,7 @@ pub mod memchr; issue = "none", reason = "exposed from core to be reused in std;" )] +#[doc(hidden)] pub mod sort; mod ascii; From 613155c96ab32daf12514e76476132e6b84adaf8 Mon Sep 17 00:00:00 2001 From: Lukas Bergdoll Date: Sat, 3 Aug 2024 15:10:27 +0200 Subject: [PATCH 5/5] Apply review comments to PartialOrd section --- library/alloc/src/slice.rs | 14 +++++++------- library/core/src/slice/mod.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 21270dbed0c61..aaa6a2abbd904 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -189,14 +189,14 @@ impl [T] { /// [`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 + /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] require + /// additional precautions. For example, `f32::NAN != f32::NAN`, which doesn't fulfill the + /// reflexivity requirement of [`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())`. + /// order] users can sort slices containing floating-point values. Alternatively, if all values + /// in the slice are guaranteed to be in a subset for which [`PartialOrd::partial_cmp`] forms a + /// [total order], it's possible to sort the slice with `sort_by(|a, b| + /// a.partial_cmp(b).unwrap())`. /// /// # Current implementation /// diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 862a18c3f4a4f..e75a9a88045e9 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -2890,14 +2890,14 @@ impl [T] { /// 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 + /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] require + /// additional precautions. For example, `f32::NAN != f32::NAN`, which doesn't fulfill the + /// reflexivity requirement of [`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())`. + /// [total order] users can sort slices containing floating-point values. Alternatively, if all + /// values in the slice are guaranteed to be in a subset for which [`PartialOrd::partial_cmp`] + /// forms a [total order], it's possible to sort the slice with `sort_unstable_by(|a, b| + /// a.partial_cmp(b).unwrap())`. /// /// # Current implementation ///