Skip to content

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Oct 22, 2016

The C++11 atomic memory model defines a memory_order_consume ordering which is generally equivalent to memory_order_acquire but can allow better code generation by avoiding memory barrier instructions. Most compilers (including LLVM) currently do not implement this ordering directly and instead treat it identically to memory_order_acquire, including adding a memory barrier instruction.

There is currently work to support consume ordering in compilers, and it would be a shame if Rust did not support this. This PR therefore reserves a __Nonexhaustive variant in Ordering so that adding a new ordering is not a breaking change in the future.

This is a [breaking-change] since it disallows exhaustive matching on Ordering, however a search of all Rust code on Github shows that there is no code that does this. This makes sense since Ordering is typically only used as a parameter to an atomic operation.

@rust-highfive
Copy link
Contributor

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 22, 2016
@hanna-kruppe
Copy link
Contributor

If the purpose of the PR is to disallow exhaustive matching on Ordering, perhaps the variant should not be named after one specific potential addition — we'd presumably want to keep preventing exhaustive matching if Consume is added in the future.

@alexcrichton
Copy link
Member

If we were to do this now I'd prefer to instead just add something like __Nonexhaustive rather than __Consume. No one should ever exhaustively match on this enum!

I'd prefer to not embroil this future-proofing with contention over the consume memory ordering, so perhaps we could just do that piece?

@Amanieu
Copy link
Member Author

Amanieu commented Oct 23, 2016

I renamed __Consume to __Nonexhaustive.

@Amanieu Amanieu changed the title Reserve Ordering::Consume for future extension Prevent exhaustive matching of Ordering to allow for future extension Oct 23, 2016
@alexcrichton
Copy link
Member

Ok, starting a crater run for this.

@alexcrichton alexcrichton added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 23, 2016
@alexcrichton
Copy link
Member

Crater reports shows one regression, but it looks to be suprious.

@rfcbot fcp merge

@alexcrichton alexcrichton removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 24, 2016
@rfcbot
Copy link

rfcbot commented Oct 24, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot
Copy link

rfcbot commented Oct 31, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Oct 31, 2016

📌 Commit 5a2fb88 has been approved by alexcrichton

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Nov 2, 2016
Prevent exhaustive matching of Ordering to allow for future extension

The C++11 atomic memory model defines a `memory_order_consume` ordering which is generally equivalent to `memory_order_acquire` but can allow better code generation by avoiding memory barrier instructions. Most compilers (including LLVM) currently do not implement this ordering directly and instead treat it identically to `memory_order_acquire`, including adding a memory barrier instruction.

There is currently [work](http://open-std.org/Jtc1/sc22/wg21/docs/papers/2016/p0098r1.pdf) to support consume ordering in compilers, and it would be a shame if Rust did not support this. This PR therefore reserves a `__Nonexhaustive` variant in `Ordering` so that adding a new ordering is not a breaking change in the future.

This is a [breaking-change] since it disallows exhaustive matching on `Ordering`, however a search of all Rust code on Github shows that there is no code that does this. This makes sense since `Ordering` is typically only used as a parameter to an atomic operation.
bors added a commit that referenced this pull request Nov 2, 2016
Rollup of 10 pull requests

- Successful merges: #37351, #37405, #37473, #37482, #37488, #37498, #37502, #37513, #37517, #37523
- Failed merges: #37521
@bors bors merged commit 5a2fb88 into rust-lang:master Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants