-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Improve docs around Once::call_once_force
and OnceState
.
#45429
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
Conversation
r? @BurntSushi (rust_highfive has picked a reviewer for you, use r? to override) |
/// }); | ||
/// ``` | ||
/// | ||
/// An unpoisoned `Once`: |
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.
yes. it is a word
be053c4
to
3b5d03b
Compare
anyone from @rust-lang/docs wanna read this over? |
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.
Seems good, but i have a nit and a broader question.
src/libstd/sync/once.rs
Outdated
/// State yielded to the [`call_once_force`] method which can be used to query | ||
/// whether the [`Once`] was previously poisoned or not. | ||
/// State yielded to [`call_once_force`]’s closure parameter. The state can be | ||
/// used to query the poisoned status of the [`Once`]. |
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.
Might prefer "poison status" here, since that feels like it would better imply "the status being poisoned" rather than "the status which has been poisoned". Tiniest nit though.
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.
sounds reasonable! addressed in 8aad14c
src/libstd/sync/once.rs
Outdated
/// If this `Once` has been poisoned (some initialization panicked) then | ||
/// this function will continue to attempt to call initialization functions | ||
/// until one of them doesn't panic. | ||
/// until one of them doesn't panic. Upon an initialization not panicking, |
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.
"until one of them doesn't panic" feels weird to me. Does call_once_force
continuously call its closure until it doesn't panic, or does it just suppress panics? That this paragraph references "initialization functions" also feels weird in isolation, since call_once
/call_once_panic
only takes one closure at a time.
Looking at the rest of the Once
docs, it feels like a better explanation would involve calling out the states where the closure will and will not run: If the Once
is uninitialized, then the closure will run regardless. If it's poisoned, then call_once
will just panic, and call_once_force
will run the closure. If the Once
has successfully been initialized, then...? The closure will not run? The use of multiple initializers in the example leads me to the question of what happens when you hand a Once
multiple call_once
calls, with separate closures, if poisoning doesn't enter the picture.
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.
Does call_once_force continuously call its closure until it doesn't panic, or does it just suppress panics?
if the Once
hasn't been successfully triggered (without panic), then call_once_force
will call the closure once. otherwise, it will ignore the closure.
If the Once has successfully been initialized, then...? The closure will not run?
yep, closure won't run
The use of multiple initializers in the example leads me to the question of what happens when you hand a Once multiple call_once calls, with separate closures, if poisoning doesn't enter the picture.
if the first call_once
succeeds, all following call_once
and call_once_force
calls are no-ops
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.
does d887138 sound any clearer? thoughts?
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.
Much better, thanks! One last nit, then r=me.
src/libstd/sync/once.rs
Outdated
/// `call_one_force` will no-op. | ||
/// | ||
/// The closure `f` is yielded a [`OnceState`] structure which can be used | ||
/// to query the poisoned state of the `Once`. |
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.
"poison status"
d887138
to
aae94c7
Compare
@bors r=quietmisdreavus rollup |
📌 Commit aae94c7 has been approved by |
Improve docs around `Once::call_once_force` and `OnceState`. Added some examples, clarify behavior, etc etc. Fixes #43472.
☀️ Test successful - status-appveyor, status-travis |
Added some examples, clarify behavior, etc etc.
Fixes #43472.