improve Drop documentation #71598
improve Drop documentation #71598
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
lcnr
force-pushed
the
drop-docs
branch
2 times, most recently
from
58731b6 to
73b064e
Compare
src/libcore/ops/drop.rs
Outdated
| /// 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. |
There was a problem hiding this comment.
| /// - 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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm. I feel like the text here already says why it is special -- because it is called by the destructor.
There was a problem hiding this comment.
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::dropfor that value, if this specialDroptrait is implemented for its type.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@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. |
|
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. :) |
|
Okay I can try to be that set of eyes :) |
There was a problem hiding this comment.
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!
|
Thanks! @bors r=RalfJung,Mark-Simulacrum |
|
📌 Commit 33324f5 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 33324f5 with merge a677f67bc7d6f8fa2cf11c24cc0729e96a637664... |
|
@bors retry yield |
|
⌛ Testing commit 33324f5 with merge 8fb3f6c5c0eae27d7dfe03bdd5081c064a0cf046... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
MSYS installation failure (Cc @pietroalbini -- or should I ping the entire infra team in such cases?) @bors retry |
bors
added
S-waiting-on-bors
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
Fixes #36073
This is a continuation of #57449 and most of the work here was done by
the excellent @steveklabnik.