Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/libstd/sync/once.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,47 @@ impl Once {
});
}

/// Returns true if some `call_once` call has completed
/// successfuly. Specifically, `is_completed` will return false in
/// the following situtations:
/// * `call_once` was not called at all,
/// * `call_once` was called, but has not yet completed,
/// * the `Once` instance is poisoned
///
/// # Examples
///
/// ```
/// #![feature(once_is_completed)]
/// use std::sync::Once;
///
/// static INIT: Once = Once::new();
///
/// assert_eq!(INIT.is_completed(), false);
Copy link
Member

Choose a reason for hiding this comment

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

Think these would normally be written as assert!(INIT.is_completed()) / assert!(!INIT.is_completed())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for boolean-returning methods writing an assert_eq! in docs specifically helps a bit with readability. Here's an example from result:

https://doc.rust-lang.org/std/result/enum.Result.html#method.is_ok

/// INIT.call_once(|| {
/// assert_eq!(INIT.is_completed(), false);
/// });
/// assert_eq!(INIT.is_completed(), true);
/// ```
///
/// ```
/// #![feature(once_is_completed)]
/// use std::sync::Once;
/// use std::thread;
///
/// static INIT: Once = Once::new();
///
/// assert_eq!(INIT.is_completed(), false);
/// let handle = thread::spawn(|| {
/// INIT.call_once(|| panic!());
/// });
/// assert!(handle.join().is_err());
/// assert_eq!(INIT.is_completed(), false);
/// ```
#[unstable(feature = "once_is_completed", issue = "42")]
Copy link

Choose a reason for hiding this comment

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

Isn't this issue number wrong? Issue #42 seems totally irrelevant to this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a placeholder, there's no tracking issue for this yet

Copy link

@mzji mzji Aug 6, 2018

Choose a reason for hiding this comment

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

Well, I just found that 42 is the answer to everything, however I still hope it could be replaced by the relevant issue ^v^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As soon as @rust-lang/libs decides that we indeed want this feature and creates a tracking issue, I'll update the PR.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation!

pub fn is_completed(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Once is not generic, so this is missing an #[inline]. (@anp found a rayon bench regression likely caused by this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb is that documented in the api guidelines or somewhere similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #54662

self.state.load(Ordering::Acquire) == COMPLETE
Copy link
Member

Choose a reason for hiding this comment

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

I'm never smart enough to use any ordering besides SeqCst, but perhaps you can explain why this ordering is appropriate to whoever is clever enough to understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Reshuffled the code a bit to reuse an existing comment :-)

}

// This is a non-generic function to reduce the monomorphization cost of
// using `call_once` (this isn't exactly a trivial or small implementation).
//
Expand Down