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

Warn about multiple conflicting #[repr] hints #34623

Merged
merged 1 commit into from
Aug 31, 2016
Merged

Warn about multiple conflicting #[repr] hints #34623

merged 1 commit into from
Aug 31, 2016

Conversation

lambda-fairy
Copy link
Contributor

Closes #34622

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@Aatch
Copy link
Contributor

Aatch commented Jul 4, 2016

I wonder if this should instead be a warning instead of an error? Also, @nagisa is the use of #[repr(C, u32)] in the failing test deliberate?

@nagisa
Copy link
Member

nagisa commented Jul 4, 2016

@Aatch what do you mean/are you referring to?

@Aatch
Copy link
Contributor

Aatch commented Jul 4, 2016

@nagisa one of the tests is failing, mir_adt_construction.rs because it uses #[repr(C, u32)]. You wrote the test, so I pinged you about it.

@lambda-fairy
Copy link
Contributor Author

Okay, fixed the error in mir_adt_construction.rs. Lets see if Travis likes it now.

@nagisa
Copy link
Member

nagisa commented Jul 4, 2016

Ah, no, I don’t think it was intentional.

@lambda-fairy
Copy link
Contributor Author

lambda-fairy commented Jul 7, 2016

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.

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 8, 2016
@lambda-fairy
Copy link
Contributor Author

Just noticed this weird test failure:

failures:

---- [pretty] pretty/conflicting-repr-hints.rs stdout ----

error: pretty-printed source does not match expected source

expected:
------------------------------------------
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(dead_code)]

#[repr(C)]
enum A { A, }

#[repr(u64)]
enum B { B, }

 //~ WARNING conflicting representation hints
#[repr(C, u64)]
enum C { C, }

 //~ WARNING conflicting representation hints
#[repr(u32, u64)]
enum D { D, }

#[repr(C, packed)]
struct E(i32);

fn main() { }

------------------------------------------
actual:
------------------------------------------
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(dead_code)]

#[repr(C)]
enum A { A, }

#[repr(u64)]
enum B { B, }

//~ WARNING conflicting representation hints
#[repr(C, u64)]
enum C { C, }

//~ WARNING conflicting representation hints
#[repr(u32, u64)]
enum D { D, }

#[repr(C, packed)]
struct E(i32);

fn main() { }

------------------------------------------

Anyone know what's going on here? The test seems to expect an extra space (??)

@bors
Copy link
Contributor

bors commented Aug 25, 2016

☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts.

@lambda-fairy lambda-fairy changed the title Disallow multiple conflicting #[repr] hints Warn about multiple conflicting #[repr] hints Aug 25, 2016
@lambda-fairy
Copy link
Contributor Author

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

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.

@lambda-fairy
Copy link
Contributor Author

@eddyb I've changed the error code to the one after the last.

Should I squash the two commits, or leave them as-is?

@eddyb
Copy link
Member

eddyb commented Aug 25, 2016

@lfairy Squashing/amending is preferred in such cases.

@lambda-fairy
Copy link
Contributor Author

Sure, I've squashed it now.

@bors
Copy link
Contributor

bors commented Aug 28, 2016

☔ The latest upstream changes (presumably #36049) made this pull request unmergeable. Please resolve the merge conflicts.

@lambda-fairy
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Aug 30, 2016

☔ The latest upstream changes (presumably #36066) made this pull request unmergeable. Please resolve the merge conflicts.

@lambda-fairy
Copy link
Contributor Author

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.

@Aatch
Copy link
Contributor

Aatch commented Aug 31, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit e2e5807 has been approved by Aatch

@bors
Copy link
Contributor

bors commented Aug 31, 2016

⌛ Testing commit e2e5807 with merge b96f323...

@bors
Copy link
Contributor

bors commented Aug 31, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@lambda-fairy
Copy link
Contributor Author

Apparently someone else added a diagnostic code in the mean time 😢 Should be fixed now.

@Aatch re-r?

@eddyb
Copy link
Member

eddyb commented Aug 31, 2016

@bors r=Aatch

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit 42b75a5 has been approved by Aatch

@bors
Copy link
Contributor

bors commented Aug 31, 2016

⌛ Testing commit 42b75a5 with merge 603d9cc...

bors added a commit that referenced this pull request Aug 31, 2016
Warn about multiple conflicting #[repr] hints

Closes #34622
@bors bors merged commit 42b75a5 into rust-lang:master Aug 31, 2016
@lambda-fairy lambda-fairy deleted the repr-conflict branch August 27, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants