core: Make funnel shifts panic only if overflow checks are enabled - #161204
core: Make funnel shifts panic only if overflow checks are enabled#161204tgross35 wants to merge 2 commits into
Conversation
rustbot
added
S-waiting-on-review
|
r? @nia-e rustbot has assigned @nia-e. Use Why was this reviewer chosen?The reviewer was selected based on:
|
caf6991 to
9cbfa6f
Compare
|
See also discussion at the stabilization PR #161015. This could wait for team feedback but I expect it to be reasonably unobjectionable. |
I wasn't able to find a test that verifies different behavior based on whether or not overflow checks are available, so add one here.
Change from the existing behavior of panicking unconditionally to panicking only if debug assertions are enabled, wrapping otherwise. This is more consistent with other operations, and gives slightly better codegen [1]. Suggested in the stabilization PR, RUST-161015. [1]: https://rust.godbolt.org/z/Yz81zhbno
9cbfa6f to
fc370f6
Compare
|
Depending on the decision taken about this, we may add |
| /// | ||
| /// ## Overflow behavior | ||
| /// | ||
| /// The above panic happens only if overflow checks are enabled (default in debug mode). |
There was a problem hiding this comment.
I think it is quite confusing to first say "panics if X" and then in the next section say "actually only sometimes".
There was a problem hiding this comment.
I was mirroring what we have elsewhere, but I agree it's not ideal. I'll drop the separate section.
|
This looks good impl-wise ^^ my only comment would be that since the wrapping methods were never added even though the ACP mentions them & this PR already has code for wrapping semantics, I'd be happy to review a follow-up PR adding those as well. r=me once the docs are fixed up per the above @bors rollup |
@nia-e We have that followup PR already at #161119, which we will rebase/update once this merges. |
5 participants
Change from the existing behavior of panicking unconditionally to panicking only if overflow checks are enabled, wrapping otherwise. This is more consistent with other operations, and gives slightly better codegen https://rust.godbolt.org/z/Yz81zhbno.
Suggested in the stabilization PR at #161015 (comment).
The first commit adds some simple tests for overflow behavior that I couldn't find existing anywhere else.