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

Document drop more. #42159

Merged
merged 5 commits into from
May 25, 2017
Merged

Document drop more. #42159

merged 5 commits into from
May 25, 2017

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented May 22, 2017

Adds two examples to Drop and describes the recursive drop on types that contain fields.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@Havvy
Copy link
Contributor Author

Havvy commented May 22, 2017

r? @steveklabnik

(Does this work?)

@rust-highfive rust-highfive assigned steveklabnik and unassigned brson May 22, 2017
@@ -153,6 +153,14 @@ use marker::Unsize;
/// The `Drop` trait is used to run some code when a value goes out of scope.
/// This is sometimes called a 'destructor'.
///
/// When a value goes out of scope, if it implements this trait, it will have
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit repetitive w.r.t "when a value goes out of scope".

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, as the first line is a summary, and this is the full explanation.

///
/// Because of the recursive dropping, even for types that do not implement
/// this trait, you do not need to implement this trait unless your type
/// needs its own destructor logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

"even for types that do not implement this trait, you do not need to implement this trait"

doesn't parse to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what happens when you stare at the screen for half an hour wondering what to write.

@@ -171,6 +179,44 @@ use marker::Unsize;
/// let _x = HasDrop;
/// }
/// ```
///
/// Showing the recursive nature of `Drop`. When `outer` goes out of scope, the
Copy link
Contributor

Choose a reason for hiding this comment

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

First sentence is incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only as much as the first sentence to the previous example which says "A trivial implementation of Drop".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; I think this is fine, though personally, I tend to use fragments like this for the short ones, and since there's more here, I'd probably make it a full sentence. NBD

///
/// Showing the recursive nature of `Drop`. When `outer` goes out of scope, the
/// `drop` method will be called for `Outer` and then the `drop` method for
/// `Inner` will be called. Therefore `main` prints `Dropping Outer!` and then
Copy link
Contributor

Choose a reason for hiding this comment

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

repetitive wording "the drop method will be called for Outer and then the drop method for Inner will be called."
Why not "the drop method will be called first for Outer, then for Inner"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do.

/// }
/// ```
///
/// Because variables are dropped in the reverse order they are declared,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "declared" the right term here?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@@ -153,6 +153,14 @@ use marker::Unsize;
/// The `Drop` trait is used to run some code when a value goes out of scope.
/// This is sometimes called a 'destructor'.
///
/// When a value goes out of scope, if it implements this trait, it will have
Copy link
Member

Choose a reason for hiding this comment

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

That's fine, as the first line is a summary, and this is the full explanation.

@@ -171,6 +179,44 @@ use marker::Unsize;
/// let _x = HasDrop;
/// }
/// ```
///
/// Showing the recursive nature of `Drop`. When `outer` goes out of scope, the
Copy link
Member

Choose a reason for hiding this comment

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

Yeah; I think this is fine, though personally, I tend to use fragments like this for the short ones, and since there's more here, I'd probably make it a full sentence. NBD

/// }
/// ```
///
/// Because variables are dropped in the reverse order they are declared,
Copy link
Member

Choose a reason for hiding this comment

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

yes

@steveklabnik
Copy link
Member

@bors: r+ rollup

thanks so much!

@bors
Copy link
Contributor

bors commented May 23, 2017

📌 Commit b41b294 has been approved by steveklabnik

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 23, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 24, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 24, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 24, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
bors added a commit that referenced this pull request May 24, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 25, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 25, 2017
Document drop more.

Adds two examples to Drop and describes the recursive drop on types that contain fields.
bors added a commit that referenced this pull request May 25, 2017
@bors bors merged commit b41b294 into rust-lang:master May 25, 2017
@Havvy Havvy deleted the doc-drop branch May 25, 2017 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants