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

atomic ordering docs #53106

Merged
merged 5 commits into from
Aug 9, 2018
Merged

atomic ordering docs #53106

merged 5 commits into from
Aug 9, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 6, 2018

Discussion in rust-lang/rfcs#2503 revealed that this could be improved. I hope this helps.

@rust-highfive
Copy link
Collaborator

r? @TimNN

(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 Aug 6, 2018
@RalfJung RalfJung mentioned this pull request Aug 6, 2018
Closed
ghost
ghost reviewed Aug 6, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I like these clarifications! 👍

Another thing we could do is specify which orderings are valid for each operation (in the docs of each of the methods like store, load, swap, etc.).

src/libcore/sync/atomic.rs
@@ -384,9 +419,11 @@ impl AtomicBool {
/// was updated.
///
/// `compare_and_swap` also takes an [`Ordering`] argument which describes the memory
/// ordering of this operation.
/// ordering of this operation. Notice that even when using [`AcqRel`], the operation
/// might fail and hence just perform an `Acquire` load, but not have `Release` semantics.
Copy link

Choose a reason for hiding this comment

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

This also applies to compare_exchange and compare_exchange_weak.

Copy link
Member Author

Choose a reason for hiding this comment

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

No it does not. Those have a separate argument for the ordering used in case of failure.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2018

Another thing we could do is specify which orderings are valid for each operation (in the docs of each of the methods like store, load, swap, etc.).

Hm... that would make for quite a large diff, and it is already stated in the Ordering enum. However, I also considered saying for each RMW operation that Release and Acquire actually perform a Relaxed access here. So I guess you are right.

kennytm
kennytm reviewed Aug 6, 2018
View reviewed changes
src/libcore/sync/atomic.rs Outdated
///
/// Corresponds to LLVM's [`Release`] ordering.
///
/// [`Release`]: http://llvm.org/docs/Atomics.html#release
/// [`Acquire`]: http://llvm.org/docs/Atomics.html#acquire
Copy link
Member

Choose a reason for hiding this comment

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

Could you change these links to HTTPS as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

src/libcore/sync/atomic.rs Outdated
///
/// [`Ordering`]: enum.Ordering.html
/// [`Ordering`]: enum.Ordering.html#variant.AcqRel
Copy link
Member

Choose a reason for hiding this comment

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

The label should be [`AcqRel`]

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 7, 2018

Another thing we could do is specify which orderings are valid for each operation (in the docs of each of the methods like store, load, swap, etc.).

Done. I hope I didn't miss any, nor screw up one of them.^^

@RalfJung RalfJung mentioned this pull request Aug 7, 2018
Closed
@TimNN
Copy link
Contributor

TimNN commented Aug 7, 2018

@stjepang: Since you already commented, and are much more familiar with the subject the I am, I think, could you review this PR?

ghost
ghost approved these changes Aug 8, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I've read through the whole set of changes and everything seems correct and clear. 👍

@TimNN
Copy link
Contributor

TimNN commented Aug 9, 2018

Thanks a lot, @stjepang!

@bors r=stjepang

@bors
Copy link
Contributor

bors commented Aug 9, 2018

📌 Commit 6a018a0 has been approved by stjepang

@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 Aug 9, 2018
@kennytm kennytm mentioned this pull request Aug 9, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Aug 9, 2018
@kennytm
Rollup merge of rust-lang#53106 - RalfJung:ordering, r=stjepang
db74946 
atomic ordering docs

Discussion in rust-lang/rfcs#2503 revealed that this could be improved. I hope this helps.
bors added a commit that referenced this pull request Aug 9, 2018
@bors
Auto merge of #53216 - kennytm:rollup, r=kennytm
8958ed6 
Rollup of 15 pull requests

Successful merges:

 - #52773 (Avoid unnecessary pattern matching against Option and Result)
 - #53082 (Fix doc link (again))
 - #53094 (Automatically expand section if url id point to one of its component)
 - #53106 (atomic ordering docs)
 - #53110 (Account for --remap-path-prefix in save-analysis)
 - #53116 (NetBSD: fix signedess of char)
 - #53179 (Whitelist wasm32 simd128 target feature)
 - #53183 (Suggest comma when missing in macro call)
 - #53207 (Add individual docs for rotate_{left, right})
 - #53211 ([nll] enable feature(nll) on various crates for bootstrap)
 - #53214 ([nll] enable feature(nll) on various crates for bootstrap: part 2)
 - #53215 (Slightly refactor syntax_ext/format)
 - #53217 (inline some short functions)
 - #53219 ([nll] enable feature(nll) on various crates for bootstrap: part 3)
 - #53222 (A few cleanups for rustc_target)
@bors bors merged commit 6a018a0 into rust-lang:master Aug 9, 2018
@RalfJung RalfJung mentioned this pull request Aug 15, 2018
Closed
@RalfJung RalfJung deleted the ordering branch August 16, 2018 10:53
@pnkfelix pnkfelix mentioned this pull request Oct 8, 2018
Closed
@ehuss ehuss mentioned this pull request Oct 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

Assignees

@TimNN TimNN

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.
5 participants
@RalfJung @rust-highfive @TimNN @bors @kennytm