Skip to content

Fixes alignment errors with new Rust union type#909

Merged
bors-servo merged 1 commit intorust-lang:masterfrom
bkchr:union_fix
Aug 14, 2017
Merged

Fixes alignment errors with new Rust union type#909
bors-servo merged 1 commit intorust-lang:masterfrom
bkchr:union_fix

Conversation

@bkchr
Copy link
Contributor

@bkchr bkchr commented Aug 13, 2017

This fix creates a new private field with the required aligned size. This new
private field ensures that the union has the required size.

This fixes: #908

@emilio
Copy link
Contributor

emilio commented Aug 13, 2017

Looks reasonable, but could you add a test for this, please? (or, we could merge #892 first, I guess...)

@emilio
Copy link
Contributor

emilio commented Aug 13, 2017

Also, thanks a lot! :)

@bkchr
Copy link
Contributor Author

bkchr commented Aug 13, 2017

Yeah, I would liked to add a test. But for the test we currently would need to enable --unstable-rust and that probably not works on stable? Or does this option just enables the Union?

@bors-servo
Copy link

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

@bkchr
Copy link
Contributor Author

bkchr commented Aug 14, 2017

@emilio I rebased upstream and adapted the tests expectations :) This should be enough test cases?

@emilio
Copy link
Contributor

emilio commented Aug 14, 2017

This looks great to me, thanks!

I think we should consider making this opt-in (or allowing to opt-out), since the people that check-in their bindings into their repos will break all sorts of 32-bit stuff... But that can be a followup.

I think if we don't have any test that fails without these changes we should definitely add one. I'm happy to check it in after your PR lands if you prefer.

Also, could you squash the commits, please?

Thanks again!

This fix creates a new private field with the required aligned size. This new
private field ensures that the union has the required size.
@bkchr
Copy link
Contributor Author

bkchr commented Aug 14, 2017

Yeah we had a test that failed without these changes, thanks for reminding me (https://github.com/rust-lang-nursery/rust-bindgen/pull/909/files#diff-08776cbee31094781c38902a13aad151) :D I forgot to remove the ifdef in the first place.
I also squashed the commits into one :)

@emilio
Copy link
Contributor

emilio commented Aug 14, 2017

@bors-servo r+

Neat, thank you!

@bors-servo
Copy link

📌 Commit 7603eb1 has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 7603eb1 with merge f9087f3...

bors-servo pushed a commit that referenced this pull request Aug 14, 2017
Fixes alignment errors with new Rust union type

This fix creates a new private field with the required aligned size. This new
private field ensures that the union has the required size.

This fixes: #908
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: emilio
Pushing f9087f3 to master...

@bors-servo bors-servo merged commit 7603eb1 into rust-lang:master Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong expected union size with Rust union

3 participants