Add inherent unchecked_shl, unchecked_shr to integers by clarfonthey · Pull Request #85703 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Tracking issue: #85122.
Adding more of these methods, since these are missing.
r? @scottmcm
(rust-highfive has picked a reviewer for you, use r? to override)
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 ?
Status: Awaiting decision from the relevant subteam (see the T- label).
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- label).
labels
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
unsafemethods I'd rather be extra clear.) - Since they're
unsafe, add a# Safetysection (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// SAFETYjustification on theirunsafeblock, I like seeing it put under a very explicit header.)
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
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.
Wording nit, not super important as you're already updating the docs, but noticed as I was reading.
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.
@rustbot label -S-waiting-on-author +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
This comment has been minimized.
Thanks for the updates, @clarfonthey! I've updated the tracking issue with some extra things to ponder before stabilization.
@bors r+
📌 Commit 2a40f24 has been approved by scottmcm
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
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library API team, which will review and decide on the PR/issue.