-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Warn about multiple conflicting #[repr] hints #34623
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
I wonder if this should instead be a warning instead of an error? Also, @nagisa is the use of |
@Aatch what do you mean/are you referring to? |
@nagisa one of the tests is failing, |
Okay, fixed the error in |
Ah, no, I don’t think it was intentional. |
I've switched to a warning instead. Should we do a crater run with this patch? I'm curious as to which crates make this mistake in the wild. |
Just noticed this weird test failure:
Anyone know what's going on here? The test seems to expect an extra space (??) |
☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts. |
I've rewritten the patch to use the HIR machinery. It should be ready to merge now. Thanks @eddyb for your help on IRC! |
@@ -1726,6 +1726,7 @@ fn cookie() -> ! { // error: definition of an unknown language item: `cookie` | |||
|
|||
register_diagnostics! { | |||
// E0006 // merged with E0005 | |||
E0011, // conflicting representation hints |
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.
You should pick the first available number after the highest in use.
@eddyb I've changed the error code to the one after the last. Should I squash the two commits, or leave them as-is? |
@lfairy Squashing/amending is preferred in such cases. |
Sure, I've squashed it now. |
☔ The latest upstream changes (presumably #36049) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased. |
☔ The latest upstream changes (presumably #36066) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased again. By the way: is anyone looking at this PR? It's been open for a while now, and I'd rather get it merged soon before the diagnostics system gets more churn. |
@bors r+ |
📌 Commit e2e5807 has been approved by |
⌛ Testing commit e2e5807 with merge b96f323... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
Apparently someone else added a diagnostic code in the mean time 😢 Should be fixed now. @Aatch re-r? |
@bors r=Aatch |
📌 Commit 42b75a5 has been approved by |
Warn about multiple conflicting #[repr] hints Closes #34622
Closes #34622