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.
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.
There was a problem hiding this comment.
Does the latest look better to you? I guess others have a # Panics section with only ## Overflow behavior in it.
There was a problem hiding this comment.
So now we have a "panics" section that entirely consists of an "overflow behavior" subsection? Do we have that anywhere else? It looks a bit strange IMO.
There was a problem hiding this comment.
Yes, this is what is done at https://doc.rust-lang.org/std/primitive.u32.html#panics-2 and others. I don't love it either but I guess it easy to find things toggled by overflow checks by searching "overflow behavior".
|
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. |
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
fc370f6 to
988ecca
Compare
|
I suppose with the update it's at least consistent, though there's probably room to improve these sections of the docs across the board. @bors r=nia-e |
rust-bors
Bot
added
S-waiting-on-bors
core: Make funnel shifts panic only if overflow checks are enabled 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 rust-lang#161015 (comment). The first commit adds some simple tests for overflow behavior that I couldn't find existing anywhere else.
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #161399 (comment) |
rust-bors
Bot
added
S-waiting-on-author
|
This pull request was unapproved. This PR was contained in a rollup (#161399), which was unapproved. |
6 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.