-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow "moves" out of immutable static (turn into memcpy) #13245
Conversation
Is there already a test which is ensuring that you can't move out of mutable static items? (just making sure we've covered our bases). Also, could you add a test that moves out multiple times and ensures that it's the same value each time? Currently we zero out the source on a move, so I just want to make sure that the zeroing out doesn't happen. |
It also looks like travis is showing some legitimate failures. I'd recommend being sure to run |
Yeah, I forgot that test. I fixed that test now. Regarding moving out of mutable statics: Since we can only access mutable statics in unsafe blocks, should assigning out of mutable statics be forbidden, or should it be allowed since we are in unsafe territory anyway? |
Moving out a mutable static should never be allowed, it is unsound for reasons beyond With immutable statics, you can always use a valid value because it's essentially just a bit pattern for initialization. All "moves" are essentially shallow copies which create new values. |
cc @nikomatsakis @flaper87 (I totally forgot to cc you, sorry) |
} else { | ||
bccx.span_err( | ||
cmt0.span, | ||
format!("cannot move out of mutable {}", |
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.
You can write the full message. There's no need to do bccx.cmt_to_str(cmt)
here since the only possible error in this arm is related to static items. The whole error probably fits in 1 line:
bccx.span_err(cmt0.span, format!("cannot move out of mutable static items");
@malu Hey, thanks a lot for working on this. Great job. Could you please squash those commits after addressing the comments? Thanks again |
@flaper87 @alexcrichton Thank you both for your help. I hope everything looks good now. If something isn't right, let me now! |
I think this patch looks mostly correct, but incomplete. The problem is that, currently, trans will still generate zeroing code because it will consider this access to be a move --- removing this zeroing code will basically happen as part of #5016. It might be that this patch has to wait until that goes in. |
Closing due to inactivity, but feel free to reopen with niko's comment addressed! |
Fixes #13233