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

Stabilize clamp #77872

Merged
merged 2 commits into from
Nov 22, 2020
Merged

Stabilize clamp #77872

merged 2 commits into from
Nov 22, 2020

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Oct 12, 2020

Tracking issue: #44095

Clamp has been merged and unstable for about a year and a half now. How do we feel about stabilizing this?

@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

(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 Oct 12, 2020
@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 Oct 12, 2020
@Xaeroxe Xaeroxe mentioned this pull request Oct 12, 2020
3 tasks
@KodrAus
Copy link
Contributor

KodrAus commented Nov 6, 2020

This seems reasonable to me!

This stabilizes the following methods on Ord, as well as f32 and f64:

pub trait Ord {
    fn clamp(self, min: Self, max: Self) -> Self { .. }
}

impl f32 {
    pub fn clamp(self, min: f32, max: f32) -> f32;
}

impl f64 {
    pub fn clamp(self, min: f64, max: f64) -> f64;
}

We need to provide inherent methods on f32 and f64 because they don't implement the Ord trait. I don't think we have any plans to implement Ord for floating points directly, but do have some total ordering methods that showed up recently.

The methods were originally RFC'd here.

There was also an FCP open here previously that went stale some time ago. It was suggested we just kick off a new one since the shape of Libs has changed a fair bit in the intervening years.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 6, 2020

Team member @KodrAus 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 Nov 6, 2020
@scottmcm
Copy link
Member

scottmcm commented Nov 8, 2020

We need to provide inherent methods on f32 and f64 because they don't implement the Ord trait. I don't think we have any plans to implement Ord for floating points directly, but do have some total ordering methods that showed up recently.

There's Ord::min and f32::min already, so I don't think having Ord::clamp and f32::clamp adds any new hazards here.

@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 Nov 8, 2020
@rfcbot
Copy link

rfcbot commented Nov 8, 2020

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. 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 Nov 18, 2020
@rfcbot
Copy link

rfcbot commented Nov 18, 2020

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.

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Nov 19, 2020
@scottmcm
Copy link
Member

The code changes look good to me, so with the FCP completed without controversy,
r? @scottmcm
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 22, 2020

📌 Commit fb6ceac has been approved by scottmcm

@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 Nov 22, 2020
@bors
Copy link
Contributor

bors commented Nov 22, 2020

⌛ Testing commit fb6ceac with merge c9e97885881f2547625bc416e550499ed4f243ae...

@bors
Copy link
Contributor

bors commented Nov 22, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2020
@scottmcm
Copy link
Member

fatal: unable to access 'https://github.com/BurntSushi/ripgrep/': Could not resolve host: github.com
thread 'main' panicked at 'assertion failed: status.success()', src\tools\cargotest\main.rs:96:13

@bors retry

@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 Nov 22, 2020
@bors
Copy link
Contributor

bors commented Nov 22, 2020

⌛ Testing commit fb6ceac with merge 5d5ff84...

@bors
Copy link
Contributor

bors commented Nov 22, 2020

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 5d5ff84 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 22, 2020
@bors bors merged commit 5d5ff84 into rust-lang:master Nov 22, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 22, 2020
@Xaeroxe Xaeroxe deleted the stabilize-clamp branch November 22, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.