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

Prevent exhaustive matching of Ordering to allow for future extension #37351

Merged
merged 1 commit into from
Nov 3, 2016

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
Collaborator

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
Contributor

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