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.

Sign up for GitHub

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

Jump to bottom

Replace data.clone() by Arc::clone(&data) in mutex doc. #56011

Merged
merged 1 commit into from Nov 19, 2018
Merged

Replace data.clone() by Arc::clone(&data) in mutex doc. #56011

merged 1 commit into from Nov 19, 2018

Conversation

CBenoit
Copy link
Contributor

@CBenoit CBenoit commented Nov 16, 2018

Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.
Since this clone is visible in the official documentation, I thought it could be better to use the more idiomatic version.

@CBenoit
Replace data.clone() by Arc::clone(&data) in mutex doc.
c1221e2 
Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.
@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Nov 16, 2018
@sfackler
Copy link
Member

Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.

Really? I don't think I've ever seen code that does that.

@CBenoit
Copy link
Contributor Author

CBenoit commented Nov 16, 2018 •
edited

Oh? That's what is stated in the Arc doc, as well as the Book.

We could have called a.clone() rather than Rc::clone(&a), but Rust’s convention is to use Rc::clone in
this case. The implementation of Rc::clone doesn’t make a deep copy of all the data like most types’
implementations of clone do. The call to Rc::clone only increments the reference count, which
doesn’t take much time. Deep copies of data can take a lot of time. By using Rc::clone for reference
counting, we can visually distinguish between the deep-copy kinds of clones and the kinds of clones
that increase the reference count. When looking for performance problems in the code, we only
need to consider the deep-copy clones and can disregard calls to Rc::clone.

However, I don't really know if this is a relevant change to apply everywhere in the doc, I'm just proposing.

@sfackler
Copy link
Member

r? @rust-lang/docs

QuietMisdreavus
QuietMisdreavus approved these changes Nov 16, 2018
View reviewed changes
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

I wasn't totally aware of those conventions, but i can agree with them! Thanks for updating these docs! r=me when travis is green.

@QuietMisdreavus
Copy link
Member

@bors rollup

@QuietMisdreavus
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2018

📌 Commit c1221e2 has been approved by QuietMisdreavus

@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 Nov 16, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Nov 17, 2018
@kennytm
Rollup merge of rust-lang#56011 - CBenoit:master, r=QuietMisdreavus
577c360 
Replace data.clone() by Arc::clone(&data) in mutex doc.

Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.
Since this clone is visible in the official documentation, I thought it could be better to use the more idiomatic version.
@kennytm kennytm mentioned this pull request Nov 17, 2018
Closed
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 18, 2018
@pietroalbini
Rollup merge of rust-lang#56011 - CBenoit:master, r=QuietMisdreavus
db982d7 
Replace data.clone() by Arc::clone(&data) in mutex doc.

Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.
Since this clone is visible in the official documentation, I thought it could be better to use the more idiomatic version.
@pietroalbini pietroalbini mentioned this pull request Nov 18, 2018
Merged
bors added a commit that referenced this pull request Nov 19, 2018
@bors
Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
d87625b 
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55935 (appveyor: Use VS2017 for all our images)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)

Failed merges:

r? @ghost
kennytm pushed a commit to pietroalbini/rust that referenced this pull request Nov 19, 2018
@pietroalbini @kennytm
Rollup merge of rust-lang#56011 - CBenoit:master, r=QuietMisdreavus
05ae505 
Replace data.clone() by Arc::clone(&data) in mutex doc.

Arc::clone(&from) is considered as more idiomatic because it conveys more explicitly the meaning of the code.
Since this clone is visible in the official documentation, I thought it could be better to use the more idiomatic version.
bors added a commit that referenced this pull request Nov 19, 2018
@bors
Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
9e8a982 
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor useful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)
 - #56059 (Increase `Duration` approximate equal threshold to 1us)
@bors bors merged commit c1221e2 into rust-lang:master Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuietMisdreavus QuietMisdreavus QuietMisdreavus approved these changes

Assignees
No one assigned
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@CBenoit @rust-highfive @sfackler @QuietMisdreavus @bors