Make Box<dyn FnOnce> respect self alignment #71170
Merged
Conversation
This file contains hidden or 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 Contributor
nikomatsakis left a comment
nikomatsakis left a comment There was a problem hiding this comment.
Also, needs a test!
spastorino force-pushed the dyn-fnonce-alignment branch from da12757 to d8549c0 Compare 
oli-obk reviewed Apr 15, 2020
Member Author
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 Member Author
| This is still not working :/ |
Contributor
| 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 Contributor
| 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 Member Author
| @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
Contributor
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.
Contributor
There was a problem hiding this comment.
But I suppose it doesn't hurt.
Contributor
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.
Contributor
There was a problem hiding this comment.
These changes seem ok 👍
Contributor
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 Contributor
| 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 
Contributor
| @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
Contributor
| @bors r+ |
Collaborator
| 📌 Commit 4e53a9a has been approved by |
bors added S-waiting-on-bors 
Merged
Collaborator
Collaborator
| ☀️ 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.
Member
| Did this also fix #61042? |
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
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