Skip to content

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

Merged
bors merged 1 commit into
rust-lang:masterfrom
Amanieu:consume
Nov 3, 2016
Merged

Prevent exhaustive matching of Ordering to allow for future extension#37351
bors merged 1 commit into
rust-lang:masterfrom
Amanieu:consume

Conversation

@Amanieu

@Amanieu Amanieu commented Oct 22, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

r? @sfackler

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

@sfackler sfackler added the T-libs-api [DEPRECATED; DO NOT USE] label Oct 22, 2016
@hanna-kruppe

Copy link
Copy Markdown
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
Copy Markdown
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

Amanieu commented Oct 23, 2016

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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

rfcbot commented Oct 24, 2016

Copy link
Copy Markdown

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

rfcbot commented Oct 31, 2016

Copy link
Copy Markdown

🔔 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
Copy Markdown
Member

@bors: r+

@bors

bors commented Oct 31, 2016

Copy link
Copy Markdown
Collaborator

📌 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 [DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants