Make Arc cloning mechanics clearer in module docs #53782
Conversation
Add some more wording to module documentation regarding how
`Arc::clone()` works, as some users have assumed cloning Arc's
to work via dereferencing to inner value as follows:
use std::sync::Arc;
let myarc = Arc::new(1);
let myarcref = myarc.clone();
assert!(1 == myarcref);
Instead of the actual mechanic of referencing the existing
Arc value:
use std::sync::Arg;
let myarc = Arc::new(1);
let myarcref = myarc.clone();
assert!(myarcref == &myarc); // not sure if assert could assert this
in the real world
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/liballoc/sync.rs
Outdated
| /// a new pointer to the same value in the heap. When the last `Arc` | ||
| /// pointer to a given value is destroyed, the pointed-to value is | ||
| /// also destroyed. | ||
| /// a new pointer to the same `Arc` reference value in the heap. When the last |
There was a problem hiding this comment.
It would be more accurate to say "produces a new Arc pointing to the same value in the heap". The source might make it clearer.
There was a problem hiding this comment.
Ooh it works like that! So the internal reference is cloned. Been hearing varying descriptions on how it works. I can change it to reflect that soon. Thanks for clarifying.
src/liballoc/sync.rs
Outdated
| @@ -107,7 +107,8 @@ const MAX_REFCOUNT: usize = (isize::MAX) as usize; | |||
| /// // The two syntaxes below are equivalent. | |||
| /// let a = foo.clone(); | |||
| /// let b = Arc::clone(&foo); | |||
| /// // a and b both point to the same memory location as foo. | |||
| /// // a and b both point to the same memory location where foo resides | |||
| /// // (not where the value wrapped by foo resides). | |||
There was a problem hiding this comment.
In fact a, b, and foo are all separate Arcs pointing to the memory location (where the Vec resides (okay actually a couple bytes away from it, to make room for the reference counts)). So I don't think this change is necessary.
There was a problem hiding this comment.
As per your other comment, I agree, will remove this.
Make it clearer that `Arc::clone()` in fact creates a whole new Arc with the internal pointer pointing to the same location as the source Arc.
| /// a new `Arc` instance, which points to the same value on the heap as the | ||
| /// source `Arc`, while increasing a reference count. When the last `Arc` | ||
| /// pointer to a given value is destroyed, the pointed-to value is also | ||
| /// destroyed. |
src/liballoc/sync.rs
Outdated
| @@ -107,7 +108,7 @@ const MAX_REFCOUNT: usize = (isize::MAX) as usize; | |||
| /// // The two syntaxes below are equivalent. | |||
| /// let a = foo.clone(); | |||
| /// let b = Arc::clone(&foo); | |||
| /// // a and b both point to the same memory location as foo. | |||
| /// // a and b both point to the same memory location as foo | |||
There was a problem hiding this comment.
If this still sounds confusing you could say "a and b both point to the same memory location as foo does" or "a, b, and foo all point to the same memory location"
There was a problem hiding this comment.
Makes sense, I probably should make that change to make it even clearer. I know for a fact that folks with English as a second language do appreciate the extra wording.
|
Are there guidelines on inline documentation formatting? Seems like parts of the documentation block I've been touching here are limited to 80 characters per line while others are limited to 100 characters? EDIT: found this: https://github.com/rust-lang-nursery/fmt-rfcs/blob/master/guide/guide.md#comments |
|
As far as I know it's 100 characters per line everywhere. The automatic
tidy check will yell if you get it wrong, anyway.
…On Fri, Aug 31, 2018 at 4:28 AM Otto Rask ***@***.***> wrote:
Are there guidelines on inline documentation formatting? Seems like parts
of the documentation block I've been touching here are limited to 80
characters per line while others are limited to 100 characters?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53782 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n37Kyo04G4rn5YKqwTR39txLDuDTks5uWPORgaJpZM4WRQ6_>
.
|
|
@bors r+ rollup |
|
📌 Commit bf7e324 has been approved by |
bors
added
S-waiting-on-bors
…amertj
Make Arc cloning mechanics clearer in module docs
Add some more wording to module documentation regarding how
`Arc::clone()` works, as some users have assumed cloning Arc's
to work via dereferencing to inner value as follows:
use std::sync::Arc;
let myarc = Arc::new(1);
let myarcref = myarc.clone();
assert!(1 == myarcref);
Instead of the actual mechanic of referencing the existing
Arc value:
use std::sync::Arg;
let myarc = Arc::new(1);
let myarcref = myarc.clone();
assert!(myarcref == &myarc); // not sure if assert could assert this in the real world
Rollup of 9 pull requests Successful merges: - #53076 (set cfg(rustdoc) when rustdoc is running on a crate) - #53622 (cleanup: Add main functions to some UI tests) - #53769 (Also link Clippy repo in the CONTRIBUTING.md file) - #53774 (Add rust-gdbgui script.) - #53781 (bench: libcore: fix build failure of any.rs benchmark (use "dyn Any")) - #53782 (Make Arc cloning mechanics clearer in module docs) - #53790 (Add regression test for issue #52060) - #53801 (Prevent duplicated impl on foreign types) - #53850 (Nuke the `const_to_allocation` query)
Add some more wording to module documentation regarding how
Arc::clone()works, as some users have assumed cloning Arc'sto work via dereferencing to inner value as follows:
Instead of the actual mechanic of referencing the existing
Arc value: