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] Expands detach documentation in thread::JoinHande. #41981

Merged
merged 1 commit into from
Jun 3, 2017

Conversation

gamazeps
Copy link
Contributor

Part of #29378 .

  • Adds an example of a thread detaching.
  • Expands what detaching means.

r? @steveklabnik

@gamazeps gamazeps changed the title Expands detach documentation in thread::JoinHande. [Doc] Expands detach documentation in thread::JoinHande. May 13, 2017
/// println!("Original thread is joined.");
/// // We make sure that the child thread has time to run, before the main
/// // thread returns.
/// thread::sleep(Duration::from_millis(1000));
Copy link
Member

Choose a reason for hiding this comment

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

The example could be simplified with a plain scope or function instead of spawning two threads.

There’s no parent-child relationship between the threads in a process.

Copy link
Contributor Author

@gamazeps gamazeps May 14, 2017

Choose a reason for hiding this comment

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

The example could indeed be simplified to

fn main() {
    {
        thread::spawn(||{
            thread::sleep(Duration::from_millis(10));
            println!("hey");
        });
    }
    thread::sleep(Duration::from_millis(1000));
}

But this is not a form that I have ever encountered in real rust code, thus this example would not be very helpful.
On the other hand a thread spawning a new thread and being joined before the new thread is a behaviour that is encountered in real rust code. It is thus a bit more complex than needed, but I believe that it is also more helpful.

There are indeed no parent-child relationship between thread, this just seemed a good illustration, if you have a better name in mind I'll change for it.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the current example seems fine to me. But if improvements are possible, I'm open to it as long as it's easy to understand for readers.

Copy link
Member

Choose a reason for hiding this comment

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

I am happy with either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the child notation for spawned or associated.

On the other hand I still believe the current example to more helpful.

@gamazeps
Copy link
Contributor Author

gamazeps commented May 14, 2017

r? @rust-lang/docs

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2017
@frewsxcv frewsxcv added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label May 14, 2017
///
/// let _ = original_thread.join();
/// println!("Original thread is joined.");
/// // We make sure that the child thread has time to run, before the main
Copy link
Member

Choose a reason for hiding this comment

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

could you put an extra newline above 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

/// let _ = original_thread.join();
/// println!("Original thread is joined.");
/// // We make sure that the child thread has time to run, before the main
/// // thread returns.
Copy link
Member

Choose a reason for hiding this comment

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

and below here, please? that is, an empty ///. This feels a bit cramped.

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

/// println!("Original thread is joined.");
/// // We make sure that the child thread has time to run, before the main
/// // thread returns.
/// thread::sleep(Duration::from_millis(1000));
Copy link
Member

Choose a reason for hiding this comment

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

I am happy with either.

@gamazeps gamazeps force-pushed the thread-detach branch 2 times, most recently from 72f33b3 to c60ec47 Compare May 15, 2017 14:19
@gamazeps
Copy link
Contributor Author

Done :)

@Mark-Simulacrum
Copy link
Member

@steveklabnik I think this is ready for another review and should be ready to go.

@@ -1052,6 +1053,30 @@ impl<T> JoinInner<T> {
/// }).unwrap();
/// ```
///
/// Child being detached and outliving its parent:
///
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

This should be marked no_run so we compile the code but don't run it during tests (we don't want this test to take a full second)

see also: https://doc.rust-lang.org/nightly/book/first-edition/documentation.html#running-documentation-tests

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 the change @frewsxcv mentioned, thank you!

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

ping @gamazeps, just wanted to make sure this was still on your radar!

@gamazeps
Copy link
Contributor Author

gamazeps commented Jun 1, 2017

@alexcrichton and you were right to do so :)

Part of rust-lang#29378 .

- Adds an example of a thread detaching.
- Expands what `detaching` means.
@gamazeps
Copy link
Contributor Author

gamazeps commented Jun 2, 2017

Done :)

@frewsxcv
Copy link
Member

frewsxcv commented Jun 2, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 2, 2017

📌 Commit b76b9e1 has been approved by frewsxcv

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 2, 2017
[Doc] Expands `detach` documentation in `thread::JoinHande`.

Part of rust-lang#29378 .

- Adds an example of a thread detaching.
- Expands what `detaching` means.

r? @steveklabnik
bors added a commit that referenced this pull request Jun 2, 2017
Rollup of 10 pull requests

- Successful merges: #41981, #42225, #42310, #42319, #42335, #42343, #42355, #42360, #42370, #42372
- Failed merges:
bors added a commit that referenced this pull request Jun 2, 2017
Rollup of 10 pull requests

- Successful merges: #41981, #42225, #42310, #42319, #42335, #42343, #42355, #42360, #42370, #42372
- Failed merges:
@bors bors merged commit b76b9e1 into rust-lang:master Jun 3, 2017
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-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants