-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Improve memory ordering diagnostics #97389
Conversation
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
This is quite awesome and is something I encountered recently as well! Unfortunately don't have capacity to review rustc PRs right now. r? @estebank |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of nitpicks, but otherwise LGTM.
Co-authored-by: Esteban Kuber <[email protected]>
82b8e39
to
f107923
Compare
📌 Commit f107923 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened. |
…askrgr Rollup of 5 pull requests Successful merges: - rust-lang#97389 (Improve memory ordering diagnostics) - rust-lang#97780 (Check ADT field is well-formed before checking it is sized) - rust-lang#98530 (compiletest: add issue number param to `known-bug`) - rust-lang#98556 (Fix builds on Windows (closes rust-lang#98546)) - rust-lang#98561 (Fix spelling in SAFETY comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Before:
After:
Before this change, the compiler suggests the failure ordering is too strong and suggests choosing a weaker ordering. After this change, it instead suggests the success ordering is not strong enough, and suggests chosing a stronger one. This is more likely to be correct.
Also, before this change, the compiler suggested downgrading an invalid AcqRel failure ordering to Relaxed, without mentioning Acquire as an option.