-
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
Fix integer overflow in rotate_left #61454
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -3,4 +3,5 @@ fn main() { | |||
//~^ ERROR temporary value dropped while borrowed | |||
let y: &'static i32 = &(5_i32.rotate_right(3)); | |||
//~^ ERROR temporary value dropped while borrowed | |||
assert_eq!(0i16.rotate_left(124), 0); |
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.
Uh, this is weird. @oli-obk what kind of test is this? I thought it would test functionality for rotate in CTFE but that does not seem to be the case. Do we have any test that does that?
@lzutao in any case, we want to test functionality here, so please move the actual call to within a const
item. Something like
const X: i16 = 0i16.rotate_left(124);
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.
The above code just checks that we don't promote calls to the rotate_*
methods. The actual tests are in https://github.com/rust-lang/rust/blob/9ebf47851a357faa4cd97f4b1dc7835f6376e639/src/test/run-pass/const-int-rotate.rs
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.
Ah, thanks!
Both this test here and that one suffer for a severe lack of comments (e.g. for stuff like the ident
function).
@lzutao so please move the new test to (Oh, and a force-push does not trigger any notifications, so please write a message here when you think this is ready for another round of review. :) |
531e8f2
to
0b2e00c
Compare
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 |
@RalfJung I think this PR is ready for review again. |
This looks great, thanks a lot! @bors r+ |
📌 Commit c2e7b219b2b7714a4cfabf069c324f584139042c has been approved by |
Actually could you squash the commits and remove the "WIP" from the commit message? @bors r- |
Ping again, I squashed those two commits. |
Awesome! @bors r+ |
📌 Commit d392cb5 has been approved by |
@bors rollup |
@lzutao: 🔑 Insufficient privileges: not in try users |
☀️ Test successful - checks-travis, status-appveyor |
Closes #61406
r? @RalfJung