-
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
Change ManuallyDrop<T> to a lang item. #52711
Conversation
This comment has been minimized.
This comment has been minimized.
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 |
#[derive(Copy)] | ||
pub union ManuallyDrop<T>{ value: T } | ||
#[lang = "manually_drop"] | ||
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] |
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.
Nice that these can all just be derives, now! 👍
r=me once travis is happy |
@@ -243,7 +243,10 @@ pub fn trivial_dropck_outlives<'tcx>(tcx: TyCtxt<'_, '_, 'tcx>, ty: Ty<'tcx>) -> | |||
|
|||
ty::TyAdt(def, _) => { | |||
if def.is_union() { | |||
// Unions never run have a dtor. | |||
// Unions never have a dtor. | |||
true |
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.
A union implementing Drop
itself is not a concern here?
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.
Good question, cc @arielb1 @pnkfelix @nikomatsakis
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.
Actually, yes, I think it is a concern. :D #52786
(but the bug is already on nightly, so it's not introduced by this PR)
This makes @eddyb said he doesn't have much time to follow up on this, but I think I can take over. I should probably also add at least one test because it seems there are none? |
FWIW we would only need an FCP for the second commit, the first one can be landed independently and then union-related changes can be made in parallel with generalized to |
So shall we split the PR? |
f1bf3af
to
591eeff
Compare
PR submitted to fix link in reference: rust-lang/reference#373 |
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 |
@TimNN Bot shows empty log. |
Updated reference submodule so the links should be fixed. |
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 |
Dang, unrelated updates in the reference contain broken links. Reported as rust-lang/reference#374 EDIT: Fix submitted: rust-lang/reference#375 |
@bors r=nikomatsakis |
📌 Commit 4e6aea1 has been approved by |
Change ManuallyDrop<T> to a lang item. This PR implements the approach @RalfJung proposes in https://internals.rust-lang.org/t/pre-rfc-unions-drop-types-and-manuallydrop/8025 (lang item `struct` instead of `union`). A followup PR can easily solve #47034 as well, by just adding a few `?Sized` to `libcore/mem.rs`. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
unsized ManuallyDrop I think this matches what @eddyb had in rust-lang#52711 originally. ~~However, I have never added a `CoerceUnsized` before so I am not sure if I did this right. I copied the `unstable` attribute on the `impl` from elsewhere, but AFAIK it is useless because `impl`'s are insta-stable... so shouldn't this rather say "stable since 1.30"?~~ This is insta-stable and hence requires FCP, at least. Fixes rust-lang#47034
unsized ManuallyDrop I think this matches what @eddyb had in #52711 originally. ~~However, I have never added a `CoerceUnsized` before so I am not sure if I did this right. I copied the `unstable` attribute on the `impl` from elsewhere, but AFAIK it is useless because `impl`'s are insta-stable... so shouldn't this rather say "stable since 1.30"?~~ This is insta-stable and hence requires FCP, at least. Fixes #47034
This PR implements the approach @RalfJung proposes in https://internals.rust-lang.org/t/pre-rfc-unions-drop-types-and-manuallydrop/8025 (lang item
struct
instead ofunion
).A followup PR can easily solve #47034 as well, by just adding a few
?Sized
tolibcore/mem.rs
.r? @nikomatsakis