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 `cmp::{min_by, min_by_key, max_by, max_by_key}` #64047

Open
wants to merge 2 commits into
base: master
from

Conversation

@timvermeulen
Copy link
Contributor

commented Aug 31, 2019

This adds the following functions to core::cmp:

  • min_by
  • min_by_key
  • max_by
  • max_by_key

min_by and max_by are somewhat trivial to implement, but not entirely because min_by returns the first value in case the two are equal (and max_by the second). min and max can be implemented in terms of min_by and max_by, but not as easily the other way around.

To give an example of why I think these functions could be useful: the Iterator::{min_by, min_by_key, max_by, max_by_key} methods all currently hard-code the behavior mentioned above which is an ever so small duplication of logic. If we delegate them to cmp::{min_by, max_by} methods instead, we get the correct behavior for free. (edit: this is now included in the PR)

I added min_by_key / max_by_key for consistency's sake but I wouldn't mind removing them. I don't have a particular use case in mind for them, and min_by / max_by seem to be more useful.

Tracking issue: #64460

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 31, 2019

r? @withoutboats

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

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 7, 2019

Ping from triage
@timvermeulen
@withoutboats @cuviper Can you please review this?

Thanks.

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

☔️ The latest upstream changes (presumably #64281) made this pull request unmergeable. Please resolve the merge conflicts.

@timvermeulen timvermeulen force-pushed the timvermeulen:cmp_min_max_by branch from 349d3b7 to 8f529b8 Sep 8, 2019

@cuviper

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

To give an example of why I think these functions could be useful: the Iterator::{min_by, min_by_key, max_by, max_by_key} methods all currently hard-code the behavior mentioned above which is an ever so small duplication of logic. If we delegate them to cmp::{min_by, max_by} methods instead, we get the correct behavior for free.

Did you try making this change?

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@cuviper I did indeed, here's the diff: timvermeulen@aed2e46

It's not a huge win, but it gets rid of the stability logic. And I guess it's also nice to have them in terms of the more general fold1 rather than select_fold1.

@cuviper

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I like it! Care to add that motivation to this PR?

Fill in a tracking issue too, and I think we can land this.

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

@cuviper Thanks, I added it.

@cuviper

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

I meant to actually include the iterator commit here, but it can be a followup if you prefer.

The tracking issue number needs to be set in the #[unstable] attributes.

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Ah, I was going to submit a follow-up, but I'm happy to include it here too.

@timvermeulen timvermeulen force-pushed the timvermeulen:cmp_min_max_by branch from fce4e44 to 7217591 Sep 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.