Skip to content
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

[DOC] Improve the thread::park and thread::unpark documentation #41809

Merged
merged 8 commits into from
May 10, 2017

Conversation

gamazeps
Copy link
Contributor

@gamazeps gamazeps commented May 7, 2017

Part of #29378 .

Takes care of the documentation for park, park_duration and also improves the unpark example.

  • park should have its module documentation inlined here, and cleaned up.
  • park_timeout could use links to park.

Felix Raimundo added 4 commits May 7, 2017 13:47
- Adds an explanantion of what `park` does in the `unpark` documentation.
- Adds a link to the module doc.
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@Mark-Simulacrum
Copy link
Member

r? @steveklabnik

/// The semantics of this function are equivalent to `park()` except that the
/// thread will be blocked for roughly no longer than `ms`. This method
/// should not be used for precise timing due to anomalies such as
/// The semantics of this function are equivalent to [`park()`][[park] except
Copy link
Member

Choose a reason for hiding this comment

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

Too many [ here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks :)

/// The semantics of this function are equivalent to `park()` except that the
/// thread will be blocked for roughly no longer than `dur`. This method
/// should not be used for precise timing due to anomalies such as
/// The semantics of this function are equivalent to [`park()`][[park] except
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto. 👍

@frewsxcv frewsxcv added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label May 7, 2017
///
/// [unpark]: struct.Thread.html#method.unpark
/// In other words, each [`Thread`] acts a bit like a spinlock that can be
/// lockend and unlocked using `park` and `unpark`.
Copy link
Member

Choose a reason for hiding this comment

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

locked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So much typos, the shame is strong in me.

/// # park and unpark
///
/// Every thread is equipped with some basic low-level blocking support, via the
/// [`thread::park`][`park`] function and [`thread::Thread::unpark()`][`unpark`]
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if you are allowed to use the () here 😄

Copy link
Contributor Author

@gamazeps gamazeps May 7, 2017

Choose a reason for hiding this comment

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

It is used elsewhre and I checked that they worked, so I think I am allowed to do that.
I can be removed if you want to though.

Copy link
Member

Choose a reason for hiding this comment

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

currently, the policy is to remove the () for method/function names in docs (see also #40456), though it's not a huge deal

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 shall be done then :p

@@ -591,11 +604,10 @@ pub fn park() {
/// preemption or platform differences that may not cause the maximum
/// amount of time waited to be precisely `ms` long.
///
/// See the [module documentation][thread] for more detail.
/// See the [park documentation][park] for more detail.
Copy link
Member

@kennytm kennytm May 7, 2017

Choose a reason for hiding this comment

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

The link should point to [`park`]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, missed that one.

Copy link
Member

Choose a reason for hiding this comment

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

this still needs to be fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Part of rust-lang#29378

- Moves the module documentation into `park`.
- Add the same example as the one from `unpark` to `park`.
/// .unwrap();
///
/// // Let some time pass for the thread to be spawned.
/// thread::sleep(Duration::from_millis(10));
Copy link
Member

Choose a reason for hiding this comment

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

As per the failures on Travis (scroll near the bottom), you'll need to import Duration into scope here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Member

Choose a reason for hiding this comment

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

(this has been addressed)

@gamazeps
Copy link
Contributor Author

gamazeps commented May 7, 2017

All comments are addressed (sory for the delay I'm following the French election at the same time :p)

@carols10cents
Copy link
Member

Looks like one travis job failed due to network failure on checking out the PR, logged here #40474 (comment) and restarted the travis build.

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

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

r=me after that link gets fixed

@@ -591,11 +604,10 @@ pub fn park() {
/// preemption or platform differences that may not cause the maximum
/// amount of time waited to be precisely `ms` long.
///
/// See the [module documentation][thread] for more detail.
/// See the [park documentation][park] for more detail.
Copy link
Member

Choose a reason for hiding this comment

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

this still needs to be fixed

/// .unwrap();
///
/// // Let some time pass for the thread to be spawned.
/// thread::sleep(Duration::from_millis(10));
Copy link
Member

Choose a reason for hiding this comment

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

(this has been addressed)

bors added a commit that referenced this pull request May 9, 2017
[Doc] improve `thread::Thread` and `thread::Builder` documentations

Part of #29378

- Adds information about the stack_size when using `Builder`. This might be considered too low level, but I assume that if someone wants to create their own builder instead of using `thread::spawn` they may be interested in that info.
- Updates the `thread::Thread` structure doc, mostly by explaining how to get one, the previous example was removed because it was not related to `thread::Thread`, but rather to `thread::Builder::name`.
  Not much is present there, mostly because this API is not often used (the only method that seems useful is `unpark`, which is documented in #41809).
@frewsxcv
Copy link
Member

@bors r=steveklabnik rollup

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit afe74c3 has been approved by steveklabnik

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 10, 2017
[DOC] Improve the thread::park and thread::unpark documentation

Part of rust-lang#29378 .

Takes care of the documentation for `park`, `park_duration` and also improves the `unpark` example.

- `park should` have its module documentation inlined here, and cleaned up.
- `park_timeout` could use links to `park`.
@gamazeps
Copy link
Contributor Author

Link is fixed I think :)

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 10, 2017
[DOC] Improve the thread::park and thread::unpark documentation

Part of rust-lang#29378 .

Takes care of the documentation for `park`, `park_duration` and also improves the `unpark` example.

- `park should` have its module documentation inlined here, and cleaned up.
- `park_timeout` could use links to `park`.
@frewsxcv
Copy link
Member

Yep, looks fixed! Just waiting to be tested and merged by our infrastructure

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 10, 2017
[DOC] Improve the thread::park and thread::unpark documentation

Part of rust-lang#29378 .

Takes care of the documentation for `park`, `park_duration` and also improves the `unpark` example.

- `park should` have its module documentation inlined here, and cleaned up.
- `park_timeout` could use links to `park`.
bors added a commit that referenced this pull request May 10, 2017
Rollup of 5 pull requests

- Successful merges: #41531, #41536, #41809, #41854, #41886
- Failed merges:
@bors bors merged commit afe74c3 into rust-lang:master May 10, 2017
@gamazeps gamazeps deleted the thread-docs branch May 13, 2017 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools 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.

9 participants