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

docs improvement sync::atomic::Atomic* #44854

Closed
wants to merge 1 commit into from
Closed

docs improvement sync::atomic::Atomic* #44854

wants to merge 1 commit into from

Conversation

lorepozo
Copy link
Contributor

Addresses the Atomic* part of #29377.
r? @steveklabnik

@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 @steveklabnik (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.

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2017
@arielb1
Copy link
Contributor

arielb1 commented Sep 26, 2017

Thanks for the PR @lucasem! @steveklabnik (or someone else) should review your PR soon.

steveklabnik
steveklabnik approved these changes Sep 26, 2017
View reviewed changes
Copy link
Member

@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.

Looking good! One quick question, and then r=me.

src/libcore/sync/atomic.rs
/// integer type, [`
#[doc = $s_int_type]
/// `](
#[doc = $int_ref]
Copy link
Member

Choose a reason for hiding this comment

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

whoah, could we use this trickery to fix...

src/libcore/sync/atomic.rs
///
/// Please note that examples are shared between atomic variants of
/// primitive integer types, so it's normal that they are all
/// demonstrating [`AtomicIsize`].
Copy link
Member

Choose a reason for hiding this comment

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

... this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to replace that particular item? All of the examples are actually using AtomicIsize, so that's exactly the atomic int type that should be there.

I thought about fixing (meta-parameterizing) the examples, but that is unfortunately not doable because of the way the docs are generated (I tried!).

Copy link
Member

Choose a reason for hiding this comment

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

I meant the examples. No worries, I didn't think it was actually possible, but I didn't realize the above was possible either, so... 😄 👍

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 26, 2017

📌 Commit f7fd04a has been approved by steveklabnik

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 29, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#44854 - lucasem:atomic-docs, r=steveklabnik
7756d39 
docs improvement sync::atomic::Atomic*

Addresses the `Atomic*` part of rust-lang#29377.
r? @steveklabnik
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Sep 29, 2017
Closed
bors added a commit that referenced this pull request Sep 30, 2017
@bors
Auto merge of #44936 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
c4cca3a 
Rollup of 15 pull requests

- Successful merges: #44124, #44287, #44320, #44694, #44708, #44794, #44797, #44824, #44836, #44840, #44845, #44854, #44889, #44900, #44903
- Failed merges:
@bors
Copy link
Contributor

bors commented Sep 30, 2017

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

@Mark-Simulacrum
Copy link
Member

Merged in #44936.

@lorepozo lorepozo deleted the atomic-docs branch September 30, 2017 15:25
@GabrielMajeri GabrielMajeri mentioned this pull request Sep 9, 2018
Merged
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 5, 2018
@pietroalbini
Rollup merge of rust-lang#54078 - GabrielMajeri:expand-sync-docs, r=s…
6b24de1 
…teveklabnik

Expand the documentation for the `std::sync` module

I've tried to expand the documentation for Rust's synchronization primitives. The module level documentation explains why synchronization is required when working with a multiprocessor system,
and then links to the appropiate structure in this module.

Fixes rust-lang#29377, since this should be the last item on the checklist (documentation for `Atomic*` was fixed in rust-lang#44854, but not ticked off the checklist).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveklabnik steveklabnik steveklabnik approved these changes

Assignees

@steveklabnik steveklabnik

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lorepozo @rust-highfive @arielb1 @steveklabnik @bors @Mark-Simulacrum