-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Make saturating_add
and saturating_sub
const
functions
#58246
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks @nikic should be covered now. |
The logic here looks fine to me now. I think it would be a bit nicer to combine the code for saturating_add and saturating_sub, as they're nearly the same (the two differences are the used BinOp, and the bounds in the unsigned case). r? @oli-obk to make sure the const eval stuff is right/idiomatic. |
@@ -374,6 +374,8 @@ fn is_intrinsic_whitelisted(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> bool | |||
| "overflowing_add" // ~> .wrapping_add | |||
| "overflowing_sub" // ~> .wrapping_sub | |||
| "overflowing_mul" // ~> .wrapping_mul | |||
| "saturating_add" // ~> .saturating_add | |||
| "saturating_sub" // ~> .saturating_sub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rust-lang/lang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with the missing flags added
@bors r=oli-obk |
📌 Commit da13fbd has been approved by |
let first_term: u128 = l.to_scalar()?.to_bits(l.layout.size)?; | ||
let num_bits = l.layout.size.bits(); | ||
if l.layout.abi.is_signed() { | ||
if first_term & (1 << (num_bits-1)) == 0 { // first term is positive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it cannot overflow on the negative side because the smallest possible things we could reach here (for u8, similar for other types) are "0 - 127" and "0 + -128", and both are fine. Correct?
Awesome, thanks! @bors r=oli-obk |
📌 Commit 7854067 has been approved by |
Prioritizing because this blocks Miri from passing tests. @bors p=1 |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
You'll have to skip the test on emscripten, as it uses 128 bit integers. |
@bors r=oli-obk |
📌 Commit b04d8aa has been approved by |
Make `saturating_add` and `saturating_sub` `const` functions Fixes #58030
💔 Test failed - status-appveyor |
Make `saturating_add` and `saturating_sub` `const` functions Fixes #58030
☀️ Test successful - checks-travis, status-appveyor |
Fixes #58030