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

don't emit #[repr(align(0))] for empty unions #1595

Merged
merged 2 commits into from Jul 23, 2019

Conversation

pmarks
Copy link
Contributor

@pmarks pmarks commented Jul 20, 2019

Fixes #1565.
Add a test case for the real root cause of #1565.
Update a few apparently unrelated test cases.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good, just a tiny simplification.

src/ir/comp.rs Outdated
@@ -1078,8 +1078,13 @@ impl CompInfo {
return None;
}

if self.kind == CompKind::Union && self.fields().len() == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: No need for self.kind == CompKind::Union, since we early-return for struts before.

Also, self.fields().is_empty().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

let mut max_size = 0;
let mut max_align = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, we should probably return None if we didn't find any of the field layouts, but this is fine too I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning None if none the field fields have a layout cause theses tests to fail internally with 'Unable to get layout information?', so leaving this as-is for now.

header_issue_493_1_0_hpp
header_issue_493_hpp
header_transform_op_hpp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks :)

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@emilio emilio merged commit 9fe3e90 into rust-lang:master Jul 23, 2019
@pmarks pmarks deleted the pmarks/issue-1565-try2 branch July 24, 2019 22:15
LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repr(align(0)) for std::basic_string causing build error
3 participants