Skip to content

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

Merged
merged 1 commit into from
Oct 22, 2017

Conversation

frewsxcv
Copy link
Member

Added some examples, clarify behavior, etc etc.

Fixes #43472.

@rust-highfive
Copy link
Contributor

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

/// });
/// ```
///
/// An unpoisoned `Once`:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frewsxcv
Copy link
Member Author

anyone from @rust-lang/docs wanna read this over?

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2017
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a 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.

/// 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`].
Copy link
Member

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.

Copy link
Member Author

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

/// 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,
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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?

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2017
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a 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.

/// `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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"poison status"

@frewsxcv
Copy link
Member Author

@bors r=quietmisdreavus rollup

@bors
Copy link
Collaborator

bors commented Oct 22, 2017

📌 Commit aae94c7 has been approved by quietmisdreavus

@bors
Copy link
Collaborator

bors commented Oct 22, 2017

⌛ Testing commit aae94c7 with merge 8493813...

bors added a commit that referenced this pull request Oct 22, 2017
Improve docs around `Once::call_once_force` and `OnceState`.

Added some examples, clarify behavior, etc etc.

Fixes #43472.
@bors
Copy link
Collaborator

bors commented Oct 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: quietmisdreavus
Pushing 8493813 to master...

@bors bors merged commit aae94c7 into rust-lang:master Oct 22, 2017
@frewsxcv frewsxcv deleted the frewsxcv-once-docs branch October 22, 2017 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants