Skip to content

Update Arc docs to match new Rc docs #36665

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
Merged
merged 2 commits into from
Oct 7, 2016
Merged

Update Arc docs to match new Rc docs #36665

merged 2 commits into from
Oct 7, 2016

Conversation

kmcallister
Copy link
Contributor

Rc docs were updated in #36571. This applies similar changes to Arc docs.

r? @GuillaumeGomez

frewsxcv
frewsxcv approved these changes Sep 23, 2016
View reviewed changes
src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

s/of/if/

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

GuillaumeGomez
GuillaumeGomez reviewed Sep 23, 2016
View reviewed changes
src/liballoc/arc.rs Outdated
Copy link
Member

@GuillaumeGomez GuillaumeGomez Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why removing these lines? We generally have an example at the module level as well. I don't mind changing it but I don't like it to get completely removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems redundant. Isn't a link to the Arc docs good enough? Besides, the module-level docs are only visible if you're looking at the alloc crate directly and not std.

@GuillaumeGomez
Copy link
Member

Before r+, what do you think of this @steveklabnik?

steveklabnik
steveklabnik approved these changes Sep 26, 2016
View reviewed changes
src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

So, it's true that duplication isn't great. Usually, module-level docs should have an overview of the whole module, but with modules like these, it's just a container for Arc/Weak, and nothing else. As such, I think I'm fine with removing this in this instance. Especially given the alloc/std split as you mentioned.

steveklabnik
steveklabnik suggested changes Sep 26, 2016
View reviewed changes
Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! This PR is great, but I have a few nits.

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be addressed.

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

uuuuuuuuuuuugh these headings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... do you want me to leave it alone?

Copy link
Contributor

Choose a reason for hiding this comment

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

#9070 (comment)

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

these links can be changed to drop the [clone]...

Copy link
Contributor Author

@kmcallister kmcallister Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Is that so? I would have to change the link at the bottom to include the backticks, right? And that bugs me personally, although I agree it doesn't really matter.

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

.. and then put

[`clone`]

here.

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"references" is the proper term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/liballoc/arc.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit torn here. Showing the imports is a bit better, I think: it's less confusing for newer users, and makes it copy/paste able.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@kmcallister
Copy link
Contributor Author

I updated the PR. Let me know what you think, and if it's good, I will squash the Arc changes.

@bors
Copy link
Collaborator

bors commented Sep 29, 2016

☔ The latest upstream changes (presumably #36818) made this pull request unmergeable. Please resolve the merge conflicts.

@kmcallister
Copy link
Contributor Author

Rebased, and added one more link in Rc docs.

@steveklabnik steveklabnik self-assigned this Oct 4, 2016
@steveklabnik
Copy link
Contributor

@bors: r+ rollup

Thanks for your patience here @kmcallister

@bors
Copy link
Collaborator

bors commented Oct 6, 2016

📌 Commit 29d3e57 has been approved by steveklabnik

@kmcallister
Copy link
Contributor Author

Thanks @steveklabnik! A lot of the delay was on my side, anyway.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 7, 2016
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 7, 2016
@GuillaumeGomez
Rollup merge of rust-lang#36665 - kmcallister:arc-docs, r=steveklabnik
c75001b 
Update Arc docs to match new Rc docs

`Rc` docs were updated in rust-lang#36571. This applies similar changes to `Arc` docs.

r? @GuillaumeGomez
bors added a commit that referenced this pull request Oct 7, 2016
@bors
Auto merge of #37024 - GuillaumeGomez:rollup, r=GuillaumeGomez
e51190c 
Rollup of 5 pull requests

- Successful merges: #36222, #36665, #36929, #37003, #37008
- Failed merges:
@bors bors merged commit 29d3e57 into rust-lang:master Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

+2 more reviewers

@frewsxcv frewsxcv frewsxcv approved these changes

@steveklabnik steveklabnik steveklabnik approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@steveklabnik steveklabnik

@GuillaumeGomez GuillaumeGomez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kmcallister @GuillaumeGomez @bors @steveklabnik @frewsxcv