Skip to content
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

Add comparison functions that return both the min and the max value #73857

Closed
wants to merge 4 commits into from
Closed

Add comparison functions that return both the min and the max value #73857

wants to merge 4 commits into from

Conversation

timvermeulen
Copy link
Contributor

This adds the following functions to core::cmp:

  • min_max
  • min_max_by
  • min_max_by_key

Sometimes I need the minimum or maximum of two values without losing ownership of the other one. These functions make this slightly more ergonomic than doing the comparison yourself, and less error-prone if you want to match the behavior of cmp::{min, max} when the two values compare equal.

Suggestions for naming these are very welcome, I don't love these but I couldn't think of anything better.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2020
@Mark-Simulacrum
Copy link
Member

Hm. So this seems viable -- but maybe it would be better to add sorted(), sorted_by(), etc functions to arrays which have a self -> Self signature? That way we get this for "free" for any constant number of arguments.

Let's also loop in someone on the libs team ... cc @dtolnay

Usage would then be something like this:

let [min, max] = [a, b].sorted();

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2020
@timvermeulen
Copy link
Contributor Author

You raise a good point, this could be generalized further in the number of arguments. I suppose a similar argument could be made for making min and max available to non-empty arrays 🙂

I don't have very compelling use cases at hand, but iirc I only ever needed this for 2 values, not for more. The appeal of adding this for me is that It's more primitive than sorting an array, and that it's slightly more general/"pure" than cmp::{min, max}. But I'll happily defer to someone with a stronger opinion on this!

@Mark-Simulacrum
Copy link
Member

Let's r? @dtolnay for unstable addition approval

I personally don't care much - I guess there's probably not much use for doing this with more than two elements.

On the other hand, you can actually almost do this today, just not in one line. Adding utility methods to save a couple lines doesn't seem hugely worth it.

let mut s = [a, b];
s.sort();
let [min, max] = s;

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I think I would prefer not to take these. I don't think they meet the right bar on how widely they would be applicable vs how easy it is to achieve this behavior without new helpers in the standard library.

Thanks anyway!

@dtolnay dtolnay closed this Jul 10, 2020
@timvermeulen
Copy link
Contributor Author

Thanks for taking a look! Maybe someone else has a more compelling use case later 🙂

@timvermeulen timvermeulen deleted the cmp_min_max_pair branch July 10, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants