-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Simplify SyncOnceCell's take
and drop
.
#76640
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ use crate::{ | |
cell::{Cell, UnsafeCell}, | ||
fmt, | ||
marker::PhantomData, | ||
mem::{self, MaybeUninit}, | ||
mem::MaybeUninit, | ||
ops::{Deref, Drop}, | ||
panic::{RefUnwindSafe, UnwindSafe}, | ||
sync::Once, | ||
|
@@ -316,13 +316,7 @@ impl<T> SyncOnceCell<T> { | |
/// ``` | ||
#[unstable(feature = "once_cell", issue = "74465")] | ||
pub fn into_inner(mut self) -> Option<T> { | ||
// SAFETY: Safe because we immediately free `self` without dropping | ||
let inner = unsafe { self.take_inner() }; | ||
|
||
// Don't drop this `SyncOnceCell`. We just moved out one of the fields, but didn't set | ||
// the state to uninitialized. | ||
mem::forget(self); | ||
inner | ||
self.take() | ||
} | ||
|
||
/// Takes the value out of this `SyncOnceCell`, moving it back to an uninitialized state. | ||
|
@@ -348,22 +342,12 @@ impl<T> SyncOnceCell<T> { | |
/// ``` | ||
#[unstable(feature = "once_cell", issue = "74465")] | ||
pub fn take(&mut self) -> Option<T> { | ||
mem::take(self).into_inner() | ||
} | ||
|
||
/// Takes the wrapped value out of a `SyncOnceCell`. | ||
/// Afterwards the cell is no longer initialized. | ||
/// | ||
/// Safety: The cell must now be free'd WITHOUT dropping. No other usages of the cell | ||
/// are valid. Only used by `into_inner` and `drop`. | ||
unsafe fn take_inner(&mut self) -> Option<T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I was thinking about removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's because they didn't exist yet. :) I added |
||
// The mutable reference guarantees there are no other threads that can observe us | ||
// taking out the wrapped value. | ||
// Right after this function `self` is supposed to be freed, so it makes little sense | ||
// to atomically set the state to uninitialized. | ||
if self.is_initialized() { | ||
let value = mem::replace(&mut self.value, UnsafeCell::new(MaybeUninit::uninit())); | ||
Some(value.into_inner().assume_init()) | ||
self.once = Once::new(); | ||
// SAFETY: `self.value` is initialized and contains a valid `T`. | ||
// `self.once` is reset, so `is_initialized()` will be false again | ||
// which prevents the value from being read twice. | ||
unsafe { Some((&mut *self.value.get()).assume_init_read()) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we leave some garbage bytes inside, but they are guarded by reseted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, previously this wrote There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The disassembly of this new version is slightly shorter. It seems to make one less copy. (Specifically for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also possible that the compiler just did not realize before that it was needlessly copying uninitialized memory. |
||
} else { | ||
None | ||
} | ||
|
@@ -416,9 +400,12 @@ impl<T> SyncOnceCell<T> { | |
|
||
unsafe impl<#[may_dangle] T> Drop for SyncOnceCell<T> { | ||
fn drop(&mut self) { | ||
// SAFETY: The cell is being dropped, so it can't be accessed again. | ||
// We also don't touch the `T`, which validates our usage of #[may_dangle]. | ||
unsafe { self.take_inner() }; | ||
if self.is_initialized() { | ||
// Safety: The cell is initialized and being dropped, so it can't | ||
// be accessed again. We also don't touch the `T` other than | ||
// dropping it, which validates our usage of #[may_dangle]. | ||
unsafe { (&mut *self.value.get()).assume_init_drop() }; | ||
} | ||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.