-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Clean up OnceCell #3945
Clean up OnceCell #3945
Conversation
@hawkw are you able to take a look at this one? ❤️ |
sure, giving it a look! |
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.
this looks great --- the code is much clearer, and the docs are much better IMO. 👍
tokio/src/sync/once_cell.rs
Outdated
/// | ||
/// [`SetError::AlreadyInitializedError`]: crate::sync::SetError::AlreadyInitializedError | ||
/// [`SetError::InitializingError`]: crate::sync::SetError::InitializingError | ||
/// ['OnceCell::get_or_init`]: crate::sync::OnceCell::get_or_init | ||
pub fn set(&self, value: T) -> Result<(), SetError<T>> { | ||
if !self.initialized() { |
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.
take it or leave it: might read a little better if this was
if self.initialized() {
return Err(SetError::AlreadyInitializedError(value));
}
match self.semaphore.try_acquire() {
// ...
}
but, take it or leave it
/// | ||
/// This will deadlock if `f` tries to initialize the cell itself. | ||
/// This will deadlock if `f` tries to initialize the cell recursively. |
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.
could be nice to have an example here, but not a blocker.
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.
Well, then I would want a normal example too. We can't have the only example be how to do it incorrectly.
/// | ||
/// This will deadlock if `f` tries to initialize the cell itself. | ||
/// This will deadlock if `f` tries to initialize the cell recursively. |
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.
similarly, an example might be nice.
This cleans up the
OnceCell
a bit.