Skip to content

Commit

Permalink
Apply review comments
Browse files Browse the repository at this point in the history 
- 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"
  • Loading branch information
@Voultapher
Voultapher committed Jul 31, 2024
1 parent b014b0d commit 2188712
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 86 deletions.
88 changes: 47 additions & 41 deletions alloc/src/slice.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")]
Expand All @@ -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
///
Expand All @@ -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")]
Expand All @@ -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
///
Expand All @@ -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")]
Expand All @@ -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.
Expand All @@ -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")]
Expand Down
80 changes: 43 additions & 37 deletions core/src/slice/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand All @@ -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)
Expand All @@ -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
///
Expand All @@ -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)
Expand All @@ -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
///
Expand All @@ -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)
Expand Down Expand Up @@ -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
///
Expand All @@ -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])
Expand Down Expand Up @@ -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
///
Expand All @@ -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>(
Expand Down Expand Up @@ -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>(
Expand Down
Loading

0 comments on commit 2188712

Please sign in to comment.