-
Notifications
You must be signed in to change notification settings - Fork 707
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
Support deriving copy for large array #874
Conversation
tests/expectations/tests/class.rs
Outdated
@@ -178,6 +194,7 @@ fn bindgen_test_layout_WithDtor() { | |||
stringify ! ( b ) )); | |||
} | |||
#[repr(C)] | |||
#[derive(Copy)] | |||
pub struct IncompleteArrayNonCopiable { |
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.
Not sure what to do with this. Change the name?
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.
I think this should really be kept non-copiable though... That's a huge footgun, because incomplete arrays tend to be used to allocate to the end of the object, and copying it would lose the trailing information.
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.
Well, that was easier than what I'd have expected... r=me
} | ||
#[repr(C)] | ||
pub struct ST<T> { | ||
pub large_array: [T; 33usize], |
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.
Hmm, so why did this suddenly start working? There have been multiple refactors of the derive debug code, @fitzgen, do you know which one could cause this?
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.
@emilio I'm unsure what "this" that suddenly started working is? We aren't deriving Debug
here, which it seems like you're implying, so I am confused.
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.
I meant that the length check was effectively fixing that, so it eventually became unneeded, and I wondered if you knew off-hand why :)
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.
From my memory, deriving Copy
was working before bindgen
gained the ability to derive Debug
. It may as well just be that the implementor treat all derive the same, while not knowing Copy
is a special trait to the compiler in regard to large array derive. Should be able to verify this aginst the commit history, but a bit tedious...
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.
Hmm, actually...
tests/expectations/tests/class.rs
Outdated
@@ -178,6 +194,7 @@ fn bindgen_test_layout_WithDtor() { | |||
stringify ! ( b ) )); | |||
} | |||
#[repr(C)] | |||
#[derive(Copy)] | |||
pub struct IncompleteArrayNonCopiable { |
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.
I think this should really be kept non-copiable though... That's a huge footgun, because incomplete arrays tend to be used to allocate to the end of the object, and copying it would lose the trailing information.
@@ -38,7 +38,7 @@ impl <T> ::std::clone::Clone for __IncompleteArrayField<T> { | |||
} | |||
impl <T> ::std::marker::Copy for __IncompleteArrayField<T> { } | |||
#[repr(C, packed)] | |||
#[derive(Debug, Default, Copy)] |
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.
Actually bindgen
derived Copy
for incomplete array even before this change. The reason some of the structs with incomplete array in previous tests have not been deriving Copy
is that they also contain large arrays...
This will disallow previous allowed deriving of Copy
though if it contains an incomplete array.
So this pull requests now actually makes two conceptually different changes.
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.
Then this was a pretty egregious bug; pretty sad we didn't catch that :(
Definitely shouldn't be deriving Copy
for incomplete arrays, as we have no idea what the correct thing to do in the scenario is...
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.
Yeah, we could discern incomplete arrays vs zero-sized arrays that get used as a field separator, but seems safer to just avoid it for now.
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.
I think before variable length array (the name used for incomplete array) gets into C99, zero-sized array is used as a hack to achieve the same effect? If so, maybe we should not assume zero-sized array is not used as incomplete array?
@bors-servo r+ |
📌 Commit 06b3893 has been approved by |
Support deriving copy for large array Fixes #56
☀️ Test successful - status-travis |
Thanks, btw! |
Pleasure! |
Fixes #56