Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove unnecessary uses of #[no_move] #8286
Conversation
highfive
commented
Oct 31, 2015
|
|
|
|
|
Oh... huh. I completely missed that; I expected all the lints to be in components/plugins/lints. In that case, it might make sense to leave RootedVec as-is for the moment. The other changes still make sense, I think? |
eefriedman
changed the title
|
I'm wary of changing RootedVec without a discussion with nox and Ms2ger, it's a very delicate API. Removing the no_move from bindings makes sense though, not sure how that got there. cc @nox |
|
RootedVec definitely still needs no_move, cf. #8159 (comment) where I tried to move one and got a panic at runtime. |
|
@Manishearth It's weird that tenacious didn't catch that specific piece of code, btw. |
|
@nox tenacious only handles some cases, the issue is that Rust optimizes most moves into noops, but not always (actual in-memory move and compile-time "moved variable" are different), so to keep the lint from being extremely noisy the cases where it's usually optimized aren't really handled. |
|
@Manishearth Yeah I understand that, but is RVO ever done to return from plain old code blocks, as opposed to function blocks? |
|
It should be. It's easy enough to check for though, just add a check_block to no_move that ensures the retval isn't a nomove type. |
|
I'm pretty sure the version of RootedVec without no_move in this PR is safe... it's just a bit slower because of the extra heap allocation. |
|
r? @nox |
|
I would rather keep RootedVec no_move and not behind a Box. |
|
@nox Updated. |
|
You forgot to remove "Misc cleanups around RootedVec." from the commit message. Otherwise, LGTM. For reference, codegen used to mark dictionary structures as not movable because they can contain roots, which used to not be movable either. -S-awaiting-review +S-needs-code-changes Reviewed 5 of 5 files at r1. Comments from the review on Reviewable.io |
The patch makes RootCollection a bit safer by making the StackRootTLS hold it in place. The use of no_move in CodeGenRust was leftover from when roots couldn't be moved.
|
@bors-servo r=nox |
|
|
Remove unnecessary uses of #[no_move] The patch makes RootCollection a bit safer by making the StackRootTLS hold it in place. RootedVec was doing an extremely delicate dance and just hoping nobody messed it up; switch to a Box to be safe. CodeGenRust seemed to be using no_move for no particularly good reason. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8286) <!-- Reviewable:end -->
|
|
|
@bors-servo retry (weird one-time Android build failure) |
Remove unnecessary uses of #[no_move] The patch makes RootCollection a bit safer by making the StackRootTLS hold it in place. RootedVec was doing an extremely delicate dance and just hoping nobody messed it up; switch to a Box to be safe. CodeGenRust seemed to be using no_move for no particularly good reason. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8286) <!-- Reviewable:end -->
|
|
|
|
eefriedman commentedOct 31, 2015
The patch makes RootCollection a bit safer by making the StackRootTLS hold
it in place.
RootedVec was doing an extremely delicate dance and just hoping nobody
messed it up; switch to a Box to be safe.
CodeGenRust seemed to be using no_move for no particularly good reason.