Add min and max methods to Ord and PartialOrd

[HeroesGrave]

I would've also changed the min and max functions in core::cmp, but they were marked as stable and operated by-value rather than by-reference, so I want to check if it was okay.

The new methods return the greater/lesser of two values, and if an ordering cannot be found or the two values are equal, return the value that the method was called on (self).

As for the min/max functions, would it be a good idea to a) change them to use the min/max methods provided from PartialOrd, and b) only require PartialOrd rather than Ord. Or should they just be left alone as they are "stable".

[huonw]

The nature of PartialOrd means it is possible for a.max(&b) to be different to b.max(&a) (e.g. a = std::f64::NAN, b = 0.0); this applies to the freestanding functions.

This seems like a strong negative.

[HeroesGrave]

I was aware of that issue, which is why I added the doc comments, but perhaps it should return an Option<&Self> instead?

My reasoning was that when you compare identical numbers of some kind, you still want a return value, and returning the first value (self) seemed like a sane default.

If it was to return an option instead, what would be the desired behaviour when the values are equal?

[lilyball]

Docstring formatting is wrong.

/// Returns the greater of two values.
///
/// If the values are equal or cannot be compared, the receiver is returned.

[huonw]

That is, rustdoc parses the first paragraph of the doc string as the main functionality-summary, so the extra info should be it's own paragraph.

[lilyball]

I'm with @huonw, partial ordering seems like a reason not to have this.

If we did have this, I think it would have to return Option<T>. But that's not really appropriate for a method named max().

Perhaps the right approach is to add functions partial_max() and partial_min() to std::cmp that operate on PartialOrd values and return Option<T>. Make it behave the same way as max()/min() (in that the second value is returned if they're equal).

#[inline]
pub fn partial_min<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
    match v1.partial_cmp(&v2) {
        None => None,
        Some(Less) => Some(v1),
        _ => Some(v2),
    }
}

#[inline]
pub fn partial_max<T: PartialOrd>(v1: T, v2: T) -> Option<T> {
    match v1.partial_cmp(&v2) {
        None => None,
        Some(Greater) => Some(v1),
        _ => Some(v2)
    }
}

[HeroesGrave]

Okay, I added the methods to Ord instead, and added partial methods which return an option to PartialOrd.

One thing I'm not sure about is the following:

(in that the second value is returned if they're equal).

If I called a.max(&b) and they were equal, it would make more sense to return the self object as that is what the method was called on in the first place, and so would appear to be more important.

[lilyball]

If I called a.max(&b) and they were equal, it would make more sense to return the self object as that is what the method was called on in the first place, and so would appear to be more important.

But that's not how the already-marked-#[stable] existing min()/max() work.

Also, I am not convinced it's appropriate to be adding min()/max() methods to every single orderable type. That's very frequently not correct. For example, I would expect min() on e.g.TreeSet to return the minimum contained element.

[Gankra]

+1 to what @kballard said. min/max should be free standing functions, not methods. That way they can be pulled in unambiguously when desired. Losing clean chaining is minor in comparison, imo.

[HeroesGrave]

Hmm. My original idea for this was because integer types were lacking min/max methods (whereas floats do), but then I thought that there would probably be a use case for most other Ord/PartialOrd types.

Would it be worth having min/max methods for just integers instead?

(Edit: It appears some trailing whitespace slipped in at the last minute.)