Skip to content

Behavior change for non-whitelisted types referenced by method #834

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

Closed
upsuper opened this issue Jul 21, 2017 · 4 comments
Closed

Behavior change for non-whitelisted types referenced by method #834

upsuper opened this issue Jul 21, 2017 · 4 comments

Comments

@upsuper
Copy link
Contributor

upsuper commented Jul 21, 2017

Input C/C++ Header

struct T {};
struct U {
  void test(T a);
};

Bindgen Invocation

$ bindgen input.hpp --generate types --whitelist-type U

Actual Results

#[repr(C)]
#[derive(Debug, Copy)]
pub struct U {
    pub _address: u8,
}
#[test]
fn bindgen_test_layout_U() {
    assert_eq!(::std::mem::size_of::<U>() , 1usize , concat ! (
               "Size of: " , stringify ! ( U ) ));
    assert_eq! (::std::mem::align_of::<U>() , 1usize , concat ! (
                "Alignment of " , stringify ! ( U ) ));
}
impl Clone for U {
    fn clone(&self) -> Self { *self }
}

Expected Results

Not really expected result, but before #827, it generates

#[repr(C)]
#[derive(Debug, Copy)]
pub struct T {
    pub _address: u8,
}
#[test]
fn bindgen_test_layout_T() {
    assert_eq!(::std::mem::size_of::<T>() , 1usize , concat ! (
               "Size of: " , stringify ! ( T ) ));
    assert_eq! (::std::mem::align_of::<T>() , 1usize , concat ! (
                "Alignment of " , stringify ! ( T ) ));
}
impl Clone for T {
    fn clone(&self) -> Self { *self }
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct U {
    pub _address: u8,
}
#[test]
fn bindgen_test_layout_U() {
    assert_eq!(::std::mem::size_of::<U>() , 1usize , concat ! (
               "Size of: " , stringify ! ( U ) ));
    assert_eq! (::std::mem::align_of::<U>() , 1usize , concat ! (
                "Alignment of " , stringify ! ( U ) ));
}
impl Clone for U {
    fn clone(&self) -> Self { *self }
}
@upsuper
Copy link
Contributor Author

upsuper commented Jul 21, 2017

I guess this is not really a bug, but a breaking behavior change which we should mention somewhere when people upgrade. (Do we have changelog?)

This definitely breaks Stylo, but it is probably because we are relying on something which we shouldn't rely on, and this can be easily fixed by simply whitelisting those types. So I guess this isn't really an issue.

@emilio, @fitzgen, what do you think?

@upsuper
Copy link
Contributor Author

upsuper commented Jul 21, 2017

And I actually think the new behavior is probably more desirable (because it would generate fewer types that we don't need). We should probably add this to the test suite so that we don't regress this behavior.

@emilio
Copy link
Contributor

emilio commented Jul 21, 2017

Note that I'm regressing this in #835

emilio added a commit to emilio/rust-bindgen that referenced this issue Jul 21, 2017
…ted_items() for code generation.

This standardizes the behavior change at rust-lang#834, but without regressions.

I've added a few more tests for rust-lang#833 here.
@emilio
Copy link
Contributor

emilio commented Jul 21, 2017

And my idea is to un-regress it in #836.

emilio added a commit to emilio/rust-bindgen that referenced this issue Jul 21, 2017
…ted_items() for code generation.

This standardizes the behavior change at rust-lang#834, but without regressions.

I've added a few more tests for rust-lang#833 here.
bors-servo pushed a commit that referenced this issue Jul 21, 2017
 	ir: Track the codegen-reachable items, and use it instead of whitelisted_items() for code generation.

This standardizes the behavior change at #834, but without regressions.

I've added a few more tests for #833 here.

Closes #834.
tmfink pushed a commit to tmfink/rust-bindgen that referenced this issue Aug 4, 2017
…ted_items() for code generation.

This standardizes the behavior change at rust-lang#834, but without regressions.

I've added a few more tests for rust-lang#833 here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants