Skip to content
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

Replace MaybeUninit with ManuallyDrop in Fragile #14

Merged
merged 1 commit into from
Mar 27, 2022
Merged

Replace MaybeUninit with ManuallyDrop in Fragile #14

merged 1 commit into from
Mar 27, 2022

Conversation

SabrinaJewson
Copy link
Contributor

ManuallyDrop is significantly less unsafe to work with, and does the job equally well.

@SabrinaJewson
Copy link
Contributor Author

Oh no, looks like once again someone has thought of this before me. But I will continue to argue my case:

ManuallyDrop is the correct type, because we are never dealing with uninitialized bytes of memory. We are only dealing with memory that is now not invalid but once was, but this is one of the precise use cases of ManuallyDrop. The specifics of this issue is technically an open question but I highly doubt that Rust will end up making ManuallyDrop<Box<T>> UB since this is literally one of its main use cases - that issue is more of a question of how we can make this usecase more sound under Stacked Borrows.

There are additional benefits to using ManuallyDrop<T>, namely you get to have null-pointer optimizations - Option<Fragile<T>> will only be 8 bytes instead of 16.

@danielhenrymantilla
Copy link

danielhenrymantilla commented Mar 13, 2022

FWIW, the linked issue wonders about weirder usages of MD<Box<T>>, mainly, whether something like:

let mut b = MD::new(Box::new(42_i32));
let p = &mut **b as *mut i32;
let b2 = b; // Move the `MD`-wrapped box: does this assert uniqueness / lack of aliasing of the pointer?
unsafe { *p = 0; } // Was this operation forbidden because of the above move?
println!("{}", **b2); // use the innards of moved box: it asserts validity *now*, or was it spanning all the way from `let b2`?

What this PR is doing is something way more sane / sensible / the very main raison d'être of ManuallyDrop: that of being able to take() stuff out of it, without worrying about what is left behind provided it be guaranteed never to be explicitly accessed again (since MD features no implicit accesses such as, well, drop glue).


All that to say that this PR is 💯 on point w.r.t. the suggested change

@mitsuhiko
Copy link
Owner

I'm okay with this change. I will apply this.

@mitsuhiko mitsuhiko merged commit 0a67444 into mitsuhiko:master Mar 27, 2022
@SabrinaJewson SabrinaJewson deleted the manuallydrop branch March 27, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants