Skip to content

Add max and min to Ord #42496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/doc/unstable-book/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@
- [n16](library-features/n16.md)
- [never_type_impls](library-features/never-type-impls.md)
- [nonzero](library-features/nonzero.md)
- [ord_max_min](library-features/ord-max-min.md)
- [offset_to](library-features/offset-to.md)
- [once_poison](library-features/once-poison.md)
- [oom](library-features/oom.md)
Expand Down
7 changes: 7 additions & 0 deletions src/doc/unstable-book/src/library-features/ord-max-min.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# `ord-max-min`

The tracking issue for this feature is: [#25663]

[#25663]: https://github.com/rust-lang/rust/issues/25663

------------------------
44 changes: 42 additions & 2 deletions src/libcore/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,42 @@ pub trait Ord: Eq + PartialOrd<Self> {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
fn cmp(&self, other: &Self) -> Ordering;

/// Compares and returns the maximum of two values.
///
/// Returns the second argument if the comparison determines them to be equal.
///
/// # Examples
///
/// ```
/// #![feature(ord_max_min)]
///
/// assert_eq!(2, 1.max(2));
/// assert_eq!(2, 2.max(2));
/// ```
#[unstable(feature = "ord_max_min", issue = "25663")]
fn max(self, other: Self) -> Self
where Self: Sized {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the best place to discuss this, but I think both of these could take (&self, other: &Self) and allow us to relax the Self: Sized bound -- getting an ordering doesn't require by-value access.

Copy link
Contributor Author

@Razaekel Razaekel Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to use by-value access because that's the same as what the std::cmp::max|min code below this does, and this PR is essentially moving those fns into Ord.

Plus, this isn't getting an Ordering the way cmp above is, but moving/copying both values in, comparing them, and then returning one. I think if I took (&self, other: &Self), I'd have to return a &Self, which would violate the API defined by std::cmp::max|min.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to keep them for now, but wanted to bring it up since maybe others will want to change it "while we're at it."

Copy link
Contributor Author

@Razaekel Razaekel Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an issue with it either way, I was just keeping things similar to what already existed since that results in less surprises.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that T is a more helpful return type than &T.

Also, #25663 (comment) argues for consistency with the inherent impl on fN:

I often write performance-sensitive graphics code, which I prototype on floats for ease of writing, and then where appropriate change to use integers for speed/memory efficiency. 90% of calculations just work, and max is one of the warts that doesn't.

Shame that a.min(&b) fails to compile instead of auto-deref'ing to (&a).min(&b), as it'd be an elegant way to use the impl<T:Ord> Ord for &T where borrow comparison is desirable...

if other >= self { other } else { self }
}

/// Compares and returns the minimum of two values.
///
/// Returns the first argument if the comparison determines them to be equal.
///
/// # Examples
///
/// ```
/// #![feature(ord_max_min)]
///
/// assert_eq!(1, 1.min(2));
/// assert_eq!(2, 2.min(2));
/// ```
#[unstable(feature = "ord_max_min", issue = "25663")]
fn min(self, other: Self) -> Self
where Self: Sized {
if self <= other { self } else { other }
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -678,6 +714,8 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
///
/// Returns the first argument if the comparison determines them to be equal.
///
/// Internally uses an alias to `Ord::min`.
///
/// # Examples
///
/// ```
Expand All @@ -689,13 +727,15 @@ 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 }
v1.min(v2)
}

/// Compares and returns the maximum of two values.
///
/// Returns the second argument if the comparison determines them to be equal.
///
/// Internally uses an alias to `Ord::max`.
///
/// # Examples
///
/// ```
Expand All @@ -707,7 +747,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 v2 >= v1 { v2 } else { v1 }
v1.max(v2)
}

// Implementation of PartialEq, Eq, PartialOrd and Ord for primitive types
Expand Down
10 changes: 10 additions & 0 deletions src/libcore/tests/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ fn test_mut_int_totalord() {
assert_eq!((&mut 12).cmp(&&mut -5), Greater);
}

#[test]
fn test_ord_max_min() {
assert_eq!(1.max(2), 2);
assert_eq!(2.max(1), 2);
assert_eq!(1.min(2), 1);
assert_eq!(2.min(1), 1);
assert_eq!(1.max(1), 1);
assert_eq!(1.min(1), 1);
}

#[test]
fn test_ordering_reverse() {
assert_eq!(Less.reverse(), Greater);
Expand Down
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#![feature(iter_rfind)]
#![feature(libc)]
#![feature(nonzero)]
#![feature(ord_max_min)]
#![feature(rand)]
#![feature(raw)]
#![feature(sip_hash_13)]
Expand Down