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 inherent unchecked_shl, unchecked_shr to integers #85703

Merged
merged 1 commit into from
May 29, 2021

Conversation

clarfonthey
Copy link
Contributor

Tracking issue: #85122.

Adding more of these methods, since these are missing.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(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 May 26, 2021
@scottmcm
Copy link
Member

Thanks for the PR, @clarfonthey!

The shift semantics for Rust are a bit weird, though. Do you know any situations where the masking the shift amount is a problem in practice?

I feel like someone reaching for these would often actually want things like shl n[us]w or [la]shr exact, rather than what these intrinsics actually do.

But at the same time I guess we're stuck with checked_shl looking only at the shift amount, so maybe this is the only reasonable behaviour? And then shl_nowrap and shr_exact would go with div_exact in the "well there's another category of these methods that needs a different naming pattern"?

Any thoughts on whether these should be added, @rust-lang/libs ?

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 26, 2021
@scottmcm
Copy link
Member

I thought about this some more, and came around to the idea that it's probably fine to have these in nightly because of the parallel with checked_sh[lr]. I'll let libs make the harder decision about stabilization later.

So @clarfonthey can you please:

  • Update the documentation to explicitly mention that this is not about overflow in output values. (Not that the docs hint that it could be, but for unsafe methods I'd rather be extra clear.)
  • Since they're unsafe, add a # Safety section (example) with a bullet for each condition that must be uphelp to avoid being UB. (You have it in the doc comment already, but to make it easier on the person wanting to write a // SAFETY justification on their unsafe block, I like seeing it put under a very explicit header.)

@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 May 28, 2021
@clarfonthey
Copy link
Contributor Author

clarfonthey commented May 29, 2021

Will update the docs for the methods in a bit. Also will add the # Safety notice to the other unsafe methods as they do not include it either, and I just copied their docs.

In terms of the actual behaviour, even though we're technically tabling the discussion: I strongly believe that we should keep the existing behaviour, as it matches the overflow behaviour of the operators normally. I definitely think we could add new methods in the future for what you specified, though. For example, division can always allow a nonzero remainder, checked or not, but longer-term we can add a new method to use exact_div for when the remainder is guaranteed to be zero.

Mostly just my thoughts on that.

library/core/src/num/int_macros.rs Outdated Show resolved Hide resolved
library/core/src/num/uint_macros.rs Outdated Show resolved Hide resolved
library/core/src/num/uint_macros.rs Outdated Show resolved Hide resolved
library/core/src/num/int_macros.rs Outdated Show resolved Hide resolved
@CraftSpider
Copy link
Contributor

Wording nit, not super important as you're already updating the docs, but noticed as I was reading.

@clarfonthey
Copy link
Contributor Author

Wording nit, not super important as you're already updating the docs, but noticed as I was reading.

Apologies, I just was about to push my changes that fixed that when you commented. :P

I chose to use "larger than" instead of "greater than" since that's the wording that the existing checked methods use.

@clarfonthey
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 May 29, 2021
@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

Thanks for the updates, @clarfonthey! I've updated the tracking issue with some extra things to ponder before stabilization.

@bors r+

@bors
Copy link
Contributor

bors commented May 29, 2021

📌 Commit 2a40f24 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 May 29, 2021
@bors
Copy link
Contributor

bors commented May 29, 2021

⌛ Testing commit 2a40f24 with merge 6166929...

@bors
Copy link
Contributor

bors commented May 29, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2021
@bors bors merged commit 6166929 into rust-lang:master May 29, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 29, 2021
@clarfonthey clarfonthey deleted the unchecked_shift branch May 29, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

7 participants