rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.53k stars 12.61k forks source link

Tracking Issue for `cmp::minmax{_by,_by_key}` #115939

Open WaffleLapkin opened 1 year ago

WaffleLapkin commented 1 year ago

Feature gate: #![feature(cmp_minmax)]

This is a tracking issue for core::cmp::minmax{_by,_by_key}.

Those functions take two values and return an array of [min, max]. This is essentially a sort specialized for two arguments.

Public API

// core::cmp

pub fn minmax<T>(v1: T, v2: T) -> [T; 2]
where
    T: Ord;

pub fn minmax_by<T, F>(v1: T, v2: T, compare: F) -> [T; 2]
where
    F: FnOnce(&T, &T) -> Ordering;

pub fn minmax_by_key<T, F, K>(v1: T, v2: T, f: F) -> [T; 2]
where
    F: FnMut(&T) -> K,
    K: Ord;

Steps / History

Unresolved Questions

scottmcm commented 1 year ago

Big fan. min/max always discard one of the arguments, so they're often useless with owned non-Copy things. This API fixes that by returning both.

Jules-Bertholet commented 1 year ago

Looks like minmax_by_key would have the same issue as #34162?

WaffleLapkin commented 1 year ago

@Jules-Bertholet yes, of course. and the fix would be the same (either using HKT, which is unergonomic in rust, or having a separate function for F: FnMut(&T) -> &impl ?Sized + Ord, or adding a bound on function return, such that we are adding a bound on something that is inside the binder)

timvermeulen commented 10 months ago

I knew this looked familiar 🙂 https://github.com/rust-lang/rust/pull/73857 Obviously a +1 from me, this concept is useful surprisingly often. A less practical reason why I like this is that it's the only min/max-like functionality that would work with hypothetical undroppable types.

anko commented 9 months ago

Concern: Why -> [T; 2] over -> (T, T)?

I can't find precedent in std for either, so this may become the precedent that future functions returning "a pair of Ts" would want to be consistent with.


Informal survey

I believe these results indicate -> (T, T) is preferred, especially in this context.

By the principle of least surprise, I propose -> (T, T).

scottmcm commented 9 months ago

It used to be that the tuple was far better because it worked in patterns.

Now that let [a, b] = cmp::minmax(x, y); works, though, using the array is more reasonable than it used to be.

WaffleLapkin commented 9 months ago

@anko as @scottmcm said, a lot of this is probably historic reasons — addition of matching / destructuring arrays is relatively new (checks notes, apparently it was stabilized in Rust 1.42, not that new).

I prefer and have used an array in those signatures, because I think it's nicer and better shows the intent.

It's nicer in the sense that arrays are more powerful than tuples — since they are homogenous they allow for more operations. Even though I think the result of this expression will almost always be destructured immediately, it's still nicer.

It better shows the intent in the sense that the methods return not just a pair of two values, but an ordered array. Also it highlightes the similarity to the sort.

Ultimately I don't think it matters much, but IMO the array is better here.

(ps. arrays and tuples also implement into/from)

Ralith commented 9 months ago

Looking forward to having this! Wanted it today when writing an algorithm that identifies the smaller of two values and walks it towards the greater.

nmay231 commented 8 months ago

If the issue with #34162 takes longer than expected to resolve, could minmax (and potentially minmax_by since it's not affected) be stabilized before minmax_by_key is?

(Tbh, I'm not familiar with the requirements of stabilization)

WaffleLapkin commented 8 months ago

@nmay231 that is possible, we do partial stabilization all the time.

notme4 commented 8 months ago

It better shows the intent in the sense that the methods return not just a pair of two values, but an ordered array.

I disagree, an array is a (possibly unordered) collection of a single kind of thing, whereas a tuple is an ordered collection of (possibly different kinds of) things, so a tuple shows intent better. Also min and max are opposites so they are arguably not even really the same kind of thing, making an array an even weaker option.

That being said the ordering of min and max is arbitrary and they have clear names, so returning a struct could be even clearer. The problem with that being then you have a struct cluttering the namespace.

bend-n commented 5 months ago

@notme4 how are tuples any more ordered than arrays?