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

improve Drop documentation #71598

Merged
merged 1 commit into from
May 8, 2020
Merged

improve Drop documentation #71598

merged 1 commit into from
May 8, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 26, 2020

Fixes #36073

This is a continuation of #57449 and most of the work here was done by
the excellent @steveklabnik.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2020
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
@lcnr lcnr force-pushed the drop-docs branch 2 times, most recently from 58731b6 to 73b064e Compare May 3, 2020 08:58
src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
/// Refer to [the chapter on `Drop` in *The Rust Programming Language*][book]
/// for some more elaboration.
/// This destructor consists of two components:
/// - A call to `Drop::drop` for that value, in case `Drop` is implemented for its type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// - A call to `Drop::drop` for that value, in case `Drop` is implemented for its type.
/// - A call to `Drop::drop` for that value, in case the special `Drop` trait is implemented for its type.

This is the first time Drop is mentioned, might be a good idea to introduce it as a "special trait"?

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 don't like special here. I feel like

I am not sure what this slightly ominous announcement is telling me. It doesn't really help to understand the following text, does it?

also applies here.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. I feel like the text here already says why it is special -- because it is called by the destructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it feet like your wording may imply that there's some other Drop trait which is special.
Changed "the" to "this".

A call to Drop::drop for that value, if this special Drop trait is implemented for its type.

Copy link
Member

Choose a reason for hiding this comment

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

To me "this special drop trait" implies that there's another one ("this X and that X", making it multiple X), but "the special drop trait" pretty clearly says that there is only one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, there are other Drop traits. for me "the" does not express that we are talking about the currently documented item. When reading your recommendation I thought that there may exist another Drop trait which is special, while this Drop trait is somehow not.

trait Drop { } // this is valid and not used in destructors

src/libcore/ops/drop.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

@RalfJung -- do you want to be the reviewer for this PR? It looks like you've already invested some time into it. If not I'm happy to try to conclude the review here and approve.

@RalfJung
Copy link
Member

RalfJung commented May 6, 2020

I mostly helped refine the language and wording here. I can help finish this but would not be comfortable r+'ing alone -- we don't have a test suite for docs stuff like this so I think it is best to get as many eyes on it as we can. :)

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

LGTM (I'd word some things differently but arguably that's just taste).

Would be good to get another set of eyes on it though.

@Mark-Simulacrum
Copy link
Member

Okay I can try to be that set of eyes :)

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

This looks good to me, though I will note that it is much more conversational than I think the standard library docs are generally. I don't personally mind that too much, though I do think that we may want to stay more consistent. Regardless, this seems like a good improvement so I'm happy to approve; let's squash the commits into one and if you can fix the one nit I left then we can merge!

@Mark-Simulacrum
Copy link
Member

Thanks!

@bors r=RalfJung,Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 7, 2020

📌 Commit 33324f5 has been approved by RalfJung,Mark-Simulacrum

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 7, 2020
@bors
Copy link
Contributor

bors commented May 7, 2020

⌛ Testing commit 33324f5 with merge a677f67bc7d6f8fa2cf11c24cc0729e96a637664...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Contributor

bors commented May 7, 2020

⌛ Testing commit 33324f5 with merge 8fb3f6c5c0eae27d7dfe03bdd5081c064a0cf046...

@bors
Copy link
Contributor

bors commented May 7, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 7, 2020
@RalfJung
Copy link
Member

RalfJung commented May 7, 2020

MSYS installation failure (Cc @pietroalbini -- or should I ping the entire infra team in such cases?)

@bors retry

@bors bors added 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 7, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 8, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70733 (Add Arc::{incr,decr}_strong_count)
 - rust-lang#71598 (improve Drop documentation)
 - rust-lang#71783 (Detect errors caused by `async` block in 2015 edition)
 - rust-lang#71903 (reword "possible candidate" import suggestion)
 - rust-lang#71960 (Fix E0284 to not use incorrect wording)

Failed merges:

r? @ghost
@bors bors merged commit bd704f7 into rust-lang:master May 8, 2020
@lcnr lcnr deleted the drop-docs branch May 8, 2020 07:32
@pietroalbini
Copy link
Member

@RalfJung ping the whole team. This was fixed in #71995.

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.

std::ops::Drop documentation focuses on scope, ignores other causes of drops
9 participants