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 unchecked_{add,sub,mul} #122520

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Mar 14, 2024

Tracking issue: #85122

I think we might as well just stabilize these basic three. They're the ones that have nuw/nsw flags in LLVM.

Notably, this doesn't include the potentially-more-complex or -more-situational things like unchecked_neg or unchecked_shr that are under different feature flags.

To quote Ralf #85122 (comment),

Are there any objections to stabilizing at least unchecked_{add,sub,mul}? For those there shouldn't be any surprises about what their safety requirements are.

Semantially these are already available on stable, even in const, via checked_*+unreachable_unchecked. So IMHO we might as well just let people write them directly, rather than try to go through a let Some(x) = x.checked_add(y) else { unsafe { hint::unreachable_unchecked() }}; dance.

I added additional text to each method to attempt to better describe the behaviour and encourage wrapping_* instead.

r? rust-lang/libs-api

@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Mar 14, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 14, 2024
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the stabilize_unchecked_math_basics branch from c414a25 to a148658 Compare March 14, 2024 23:07
@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 14, 2024
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the stabilize_unchecked_math_basics branch from a148658 to e8a337f Compare March 14, 2024 23:18
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the stabilize_unchecked_math_basics branch from e8a337f to a0bf6d1 Compare March 15, 2024 00:31
@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the stabilize_unchecked_math_basics branch from a0bf6d1 to 9eb26d7 Compare March 15, 2024 01:40
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm 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 Mar 15, 2024
@scottmcm
Copy link
Member Author

Finally managed to get through CI 😅

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2024
@jhpratt
Copy link
Member

jhpratt commented Mar 15, 2024

It doesn't look like there's been an FCP for this yet. r=me once that passed, though.

@rustbot claim

@rustbot rustbot assigned jhpratt and unassigned Amanieu Mar 15, 2024
@scottmcm scottmcm added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Mar 15, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 19, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 19, 2024

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 19, 2024
@Amanieu Amanieu removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Mar 19, 2024
@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 19, 2024
@rfcbot
Copy link

rfcbot commented Mar 19, 2024

🔔 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. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 29, 2024
@rfcbot
Copy link

rfcbot commented Mar 29, 2024

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.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Mar 29, 2024
@jhpratt
Copy link
Member

jhpratt commented Mar 29, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 29, 2024

📌 Commit 50392cc has been approved by jhpratt

It is now in the queue for this repository.

@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 Mar 29, 2024
@bors
Copy link
Contributor

bors commented Mar 29, 2024

⌛ Testing commit 50392cc with merge faae5f1...

@bors
Copy link
Contributor

bors commented Mar 29, 2024

☀️ Test successful - checks-actions
Approved by: jhpratt
Pushing faae5f1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2024
@bors bors merged commit faae5f1 into rust-lang:master Mar 29, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 29, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (faae5f1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
5.3% [5.3%, 5.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 5.3% [5.3%, 5.3%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.1% [-3.1%, -3.1%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.985s -> 668.88s (0.13%)
Artifact size: 315.84 MiB -> 315.90 MiB (0.02%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2024
@scottmcm scottmcm deleted the stabilize_unchecked_math_basics branch April 24, 2024 17:25
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. 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.

None yet

9 participants