Skip to content

Stabilize cmp_min_max_by #81047

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 7, 2021
Merged

Stabilize cmp_min_max_by #81047

merged 1 commit into from
Apr 7, 2021

Conversation

glittershark
Copy link
Contributor

I would like to propose cmp::{min_by, min_by_key, max_by, max_by_key}
for stabilization.

These are relatively simple and seemingly uncontroversial functions and
have been unchanged in unstable for a while now.

Closes: #64460

@rust-highfive
Copy link
Contributor

r? @shepmaster

(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 Jan 15, 2021
@glittershark glittershark mentioned this pull request Jan 15, 2021
Closed
@glittershark
Copy link
Contributor Author

this is my first time doing a stabilization PR, so lmk if I did anything wrong here! #64460 (comment) seemed to indicate this was the proper next step.

@jonas-schievink jonas-schievink added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 15, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@glittershark glittershark force-pushed the stabilize-cmp-min-max-by branch from 876f7ae to 4d70d7c Compare January 15, 2021 17:31
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@glittershark glittershark force-pushed the stabilize-cmp-min-max-by branch from 4d70d7c to 3f85151 Compare January 15, 2021 17:41
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2021
@Dylan-DPC-zz
Copy link

@shepmaster this is waiting for your review :) thanks

@Mark-Simulacrum
Copy link
Member

r? @KodrAus for T-libs FCP

@rust-highfive rust-highfive assigned KodrAus and unassigned shepmaster Mar 8, 2021
@JohnTitor
Copy link
Member

@rust-lang/libs Could someone kick off FCP? Thanks!

@Amanieu
Copy link
Member

Amanieu commented Mar 25, 2021

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 25, 2021 •
edited by m-ou-se
Loading

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 25, 2021
JohnTitor
JohnTitor reviewed Mar 25, 2021
View reviewed changes
library/core/src/cmp.rs Outdated
/// use std::cmp;
///
/// assert_eq!(cmp::min_by(-2, 1, |x: &i32, y: &i32| x.abs().cmp(&y.abs())), 1);
/// assert_eq!(cmp::min_by(-2, 2, |x: &i32, y: &i32| x.abs().cmp(&y.abs())), -2);
/// ```
#[inline]
#[must_use]
#[unstable(feature = "cmp_min_max_by", issue = "64460")]
#[stable(feature = "cmp_min_max_by", since = "1.50.0")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[stable(feature = "cmp_min_max_by", since = "1.50.0")]
#[stable(feature = "cmp_min_max_by", since = "1.53.0")]

NOTE: This should be changed to 1.53.0 (i.e. the nightly version at that point) once FCP is done.

@BurntSushi
Copy link
Member

Why do the {min,max}_by routines take a FnOnce but the {min,max}_by_key routines take a FnMut?

@dtolnay
Copy link
Member

dtolnay commented Mar 25, 2021

{min,max}_by invoke the comparison function exactly once, on your pair of values, while {min,max}_by_key invoke the get-key function twice, once per each compared value.

@BurntSushi
Copy link
Member

@dtolnay Derp, missed that. Thanks!

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 27, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 27, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Apr 6, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 6, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 6, 2021
@glittershark
Copy link
Contributor Author

bumped version in the stable attribute to 1.52.0, which afaict is the next version - lmk if this wasn't the correct thing to do

@glittershark glittershark force-pushed the stabilize-cmp-min-max-by branch from 3f85151 to 2601fae Compare April 6, 2021 16:50
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@glittershark glittershark force-pushed the stabilize-cmp-min-max-by branch from 2601fae to 14c7f2c Compare April 6, 2021 19:24
@Amanieu
Copy link
Member

Amanieu commented Apr 7, 2021

Actually the version number should be 1.53.0 which is the current master. 1.51.0 is the current stable and 1.52.0 is the current beta.

@glittershark
Stabilize cmp_min_max_by
462f86d 
I would like to propose cmp::{min_by, min_by_key, max_by, max_by_key}
for stabilization.

These are relatively simple and seemingly uncontroversial functions and
have been unchanged in unstable for a while now.
@glittershark glittershark force-pushed the stabilize-cmp-min-max-by branch from 14c7f2c to 462f86d Compare April 7, 2021 14:29
@glittershark
Copy link
Contributor Author

Actually the version number should be 1.53.0 which is the current master. 1.51.0 is the current stable and 1.52.0 is the current beta.

done, thanks!

@Dylan-DPC-zz
Copy link

@bors r=kodraus

@bors
Copy link
Collaborator

bors commented Apr 7, 2021

📌 Commit 462f86d has been approved by kodraus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2021
@bors
Copy link
Collaborator

bors commented Apr 7, 2021

⌛ Testing commit 462f86d with merge ef2ef92...

@bors
Copy link
Collaborator

bors commented Apr 7, 2021

☀️ Test successful - checks-actions
Approved by: kodraus
Pushing ef2ef92 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 7, 2021
@bors bors merged commit ef2ef92 into rust-lang:master Apr 7, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 7, 2021
@glittershark glittershark deleted the stabilize-cmp-min-max-by branch April 7, 2021 20:29
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnTitor JohnTitor JohnTitor left review comments

Assignees

@KodrAus KodrAus

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.53.0
Development

Successfully merging this pull request may close these issues.

Tracking issue for cmp::{min_by, min_by_key, max_by, max_by_key}