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

Update to Rc and Arc documentation to favor the Rc::clone(&ptr) syntax. #42137

Merged
merged 1 commit into from
May 27, 2017

Conversation

nical
Copy link
Contributor

@nical nical commented May 21, 2017

This is a followup of the discussion in rust-lang/rfcs#1954.

The solution chosen by the core team to address the problem tackled by the the RFC was to make the function call syntax Rc::clone(&foo) the idiomatic way to clone a reference counted pointer (over the method call syntax foo.clone()).
This change updates the documentation of Rc, Arc and their respective Weak pointers to reflect this decision and bring more exposure to the existence of the function call syntax.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (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.

@nical
Copy link
Contributor Author

nical commented May 21, 2017

I am still in process of building the docs locally to check the results (taking ages on this slow computer), there are certainly some adjustments to make and I am not particularly attached to the wording itself.

I think that the most important is to bring exposure to the existence of the function call syntax and use it by default in the examples. Perhaps a more detailed explanation of the rationale behind favoring it over the method call syntax should be added to the book rather than the API docs.

cc @aturon who was the core team's "ambassador" on that RFC.

@BurntSushi BurntSushi added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 21, 2017
/// # Cloning references
///
/// Creating a new reference from an existing reference counted pointer is done using the
/// ```Clone``` trait implemented for [`Arc<T>`][`arc`] and [`Weak<T>`][`weak`].
Copy link
Member

Choose a reason for hiding this comment

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

Inline code blocks can just use one backtick, in this case:

`Clone`

It was decided in the RFC discussion rust-lang/rfcs#1954 to make the function call syntax Rc::clone(&foo) the idiomatic way to clone a reference counted pointer (over the method call syntax foo.clone(). This change updates the documentation of Rc, Arc and their respoective Weak pointers to reflect it and bring more exposure to the existence of the function call syntax.
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2017
@alexcrichton
Copy link
Member

ping @BurntSushi, mind taking a look at this PR?

@BurntSushi
Copy link
Member

@nical This looks great to me, thanks so much! @bors r+

@bors
Copy link
Contributor

bors commented May 26, 2017

📌 Commit dec23d4 has been approved by BurntSushi

@BurntSushi BurntSushi removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2017
@shepmaster shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 27, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
Update to Rc and Arc documentation to favor the Rc::clone(&ptr) syntax.

This is a followup of the discussion in rust-lang/rfcs#1954.

The solution chosen by the core team to address the problem tackled by the [the RFC](rust-lang/rfcs#1954) was to make the function call syntax Rc::clone(&foo) the idiomatic way to clone a reference counted pointer (over the method call syntax foo.clone()).
This change updates the documentation of Rc, Arc and their respective Weak pointers to reflect this decision and bring more exposure to the existence of the function call syntax.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 27, 2017
Update to Rc and Arc documentation to favor the Rc::clone(&ptr) syntax.

This is a followup of the discussion in rust-lang/rfcs#1954.

The solution chosen by the core team to address the problem tackled by the [the RFC](rust-lang/rfcs#1954) was to make the function call syntax Rc::clone(&foo) the idiomatic way to clone a reference counted pointer (over the method call syntax foo.clone()).
This change updates the documentation of Rc, Arc and their respective Weak pointers to reflect this decision and bring more exposure to the existence of the function call syntax.
bors added a commit that referenced this pull request May 27, 2017
Rollup of 10 pull requests

- Successful merges: #42103, #42137, #42162, #42167, #42175, #42207, #42217, #42246, #42249, #42251
- Failed merges:
@bors
Copy link
Contributor

bors commented May 27, 2017

⌛ Testing commit dec23d4 with merge 28fd1e5...

bors added a commit that referenced this pull request May 27, 2017
Update to Rc and Arc documentation to favor the Rc::clone(&ptr) syntax.

This is a followup of the discussion in rust-lang/rfcs#1954.

The solution chosen by the core team to address the problem tackled by the [the RFC](rust-lang/rfcs#1954) was to make the function call syntax Rc::clone(&foo) the idiomatic way to clone a reference counted pointer (over the method call syntax foo.clone()).
This change updates the documentation of Rc, Arc and their respective Weak pointers to reflect this decision and bring more exposure to the existence of the function call syntax.
@bors
Copy link
Contributor

bors commented May 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing 28fd1e5 to master...

@bors bors merged commit dec23d4 into rust-lang:master May 27, 2017
@steveklabnik
Copy link
Member

Huh, I did not know about this idiom change. @carols10cents we might have to update the new book ☹️

@carols10cents
Copy link
Member

I was poking around in some Servo code recently and saw that they favor this idiom and i like it :) Will file an issue over there if you haven't already

Centril added a commit to Centril/rust that referenced this pull request Aug 19, 2019
Remove recommendation about idiomatic syntax for Arc::clone

I believe we should not make this recommendation. I don't want to argue that `Arc::clone` is less idiomatic than `arc.clone`, but that the choice is not clear cut and that we should not be making this kind of call in the docs.

The `.clone()` form has advantages too: it is more succinct, it is more likely to be understood by beginners, and it is more uniform with other `clone` calls, indeed with most other method calls.

Whichever approach is better, I think that this discussion belongs in a style guide or textbook, rather than the library docs. We don't talk much about idiomatic code in the docs, this place is pretty exceptional.

The recommendation is also not followed in this repo. It is hard to figure out how many calls there are of the `.clone()` form, but there are 1550 uses of `Arc` and only 65 uses of `Arc::clone`. The recommendation has existed for over two years.

The recommendation was added in rust-lang#42137, as a result of rust-lang/rfcs#1954. However, note that that RFC was closed because it was not necessary to change the docs (the original RFC proposed a new function instead). So I don't think an RFC is necessary here (and I'm not trying to re-litigate the discussion on that RFC (which favoured `Arc::clone` as idiomatic) in any case).

cc @nical (who added the docs in the first place; sorry :-) )

r? @alexcrichton (or someone else on @rust-lang/libs )
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants