Make Box<dyn FnOnce> respect self alignment #71170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
from
f630df3
to
da12757
Compare
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
from
da12757
to
d8549c0
Compare
oli-obk
reviewed
Apr 15, 2020
oh, test was left untracked on my computer. |
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
3 times, most recently
from
733373a
to
cb22781
Compare
|
This is still not working :/ |
|
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 |
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
2 times, most recently
from
a94808a
to
c8e6669
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 |
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
3 times, most recently
from
4755471
to
b285807
Compare
RalfJung
reviewed
Apr 17, 2020
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
from
b285807
to
b3d4262
Compare
|
@nikomatsakis @eddyb this should be now ready for review. |
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
2 times, most recently
from
986b0e2
to
33ceb58
Compare
nikomatsakis
requested changes
Apr 17, 2020
| let local_scope = self.local_scope(); | ||
| let expr = self.hir.mirror(expr); | ||
|
|
||
| if tcx.features().unsized_locals { |
There was a problem hiding this comment.
Do we need this feature check? I guess that unsized locals, if passed, will get an error elsewhere in the compiler anyway.
There was a problem hiding this comment.
But I suppose it doesn't hurt.
There was a problem hiding this comment.
I think @eddyb had talked about possibly adding a second feature gate here.. not sure if we want to consider that in this PR? Probably not, since I imagine it may require unrelated changes elsewhere.
| --> $DIR/double-move.rs:45:9 | ||
| | | ||
| LL | let _y = *x; | ||
| | -- value moved here | ||
| LL | x.foo(); | ||
| | ^ value used here after move | ||
| | ^ value used here after partial move |
There was a problem hiding this comment.
These changes seem ok 👍
| @@ -45,12 +45,12 @@ LL | println!("{}", &y); | |||
| error[E0382]: borrow of moved value: `x` | |||
| --> $DIR/borrow-after-move.rs:39:24 | |||
| | | |||
| LL | let x = "hello".to_owned().into_boxed_str(); | |||
| | - move occurs because `x` has type `std::boxed::Box<str>`, which does not implement the `Copy` trait | |||
There was a problem hiding this comment.
So... this is a bit suboptimal, in that the move is really occurring because foo was invoked on *x -- i.e., the type str -- but the new IR makes the borrow checker see it otherwise. I'm not sure how easy/hard that would be to fix. Probably not worth worrying about, I think.
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
from
33ceb58
to
683a5fb
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 |
nikomatsakis
requested changes
Apr 18, 2020
spastorino
force-pushed
the
dyn-fnonce-alignment
branch
from
844a496
to
365807f
Compare
|
@spastorino I pushed the edits to the comments that I made, as requested. I think they're a mild improvement. |
nikomatsakis
approved these changes
Apr 20, 2020
|
@bors r+ |
|
📌 Commit 4e53a9a has been approved by |
bors
added
S-waiting-on-bors
Merged
|
☀️ Test successful - checks-azure |
This was referenced Apr 22, 2020
bjorn3
added a commit
to rust-lang/rustc_codegen_cranelift
that referenced
this pull request
Apr 22, 2020
Remove the `<Box<F> as FnOnce>::call_once` hack now that rust-lang/rust#71170 is merged.
|
Did this also fix #61042? |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #68304
r? @eddyb @nikomatsakis