Skip to content

Reintroduce bitfield accessors #567

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2017
Merged

Reintroduce bitfield accessors #567

merged 1 commit into from
Mar 9, 2017

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Mar 8, 2017

This commit reintroduces accessor methods for bitfields in the generated
bindings.

Fixes #519

r? @emilio

@highfive
Copy link

highfive commented Mar 8, 2017

warning Warning warning

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

@fitzgen
Copy link
Member Author

fitzgen commented Mar 8, 2017

Tests are failing because the travis ci gcc doesn't seem to understand static_assert:

running: "c++" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-m64" "-o" "/tmp/bindgen/debug/build/bindgen-integration-cd0402279e5ba3c7/out/cpp/Test.o" "-c" "cpp/Test.cc"
cargo:warning=In file included from cpp/Test.cc:1:0:
cargo:warning=cpp/Test.h:40:14: error: expected constructor, destructor, or type conversion before ‘(’ token
cargo:warning=cpp/Test.h:41:14: error: expected constructor, destructor, or type conversion before ‘(’ token
cargo:warning=cpp/Test.h:52:14: error: expected constructor, destructor, or type conversion before ‘(’ token
cargo:warning=cpp/Test.h:53:14: error: expected constructor, destructor, or type conversion before ‘(’ token
cargo:warning=cpp/Test.h:70:14: error: expected constructor, destructor, or type conversion before ‘(’ token
cargo:warning=cpp/Test.h:71:14: error: expected constructor, destructor, or type conversion before ‘(’ token
ExitStatus(ExitStatus(256))

@fitzgen fitzgen force-pushed the bitfield-accessors branch from beb460c to 75e9a00 Compare March 8, 2017 23:43
@fitzgen
Copy link
Member Author

fitzgen commented Mar 8, 2017

Updated with an attempt to pass -std=c++11 through to the compiler in build.rs.

@fitzgen
Copy link
Member Author

fitzgen commented Mar 9, 2017

Updated with an attempt to pass -std=c++11 through to the compiler in build.rs.

Strange... my local Fedora gcc has support for -std=c++11 but it seems that the travis ci gcc doesn't. Not sure what version of gcc it is, but the easiest thing to do here is just remove the static asserts in the C++, since they aren't essential.

@fitzgen
Reintroduce bitfield accessors
50ee737 
This commit reintroduces accessor methods for bitfields in the generated
bindings.

Fixes rust-lang#519
@fitzgen fitzgen force-pushed the bitfield-accessors branch from 75e9a00 to 50ee737 Compare March 9, 2017 17:59
@fitzgen
Copy link
Member Author

fitzgen commented Mar 9, 2017

Alright, removed the static_asserts from the C++ and CI is green. Ready when you are, @emilio :)

@emilio
Copy link
Contributor

emilio commented Mar 9, 2017

Alright, removed the static_asserts from the C++ and CI is green. Ready when you are, @emilio :)

Instead of that, I think compiling with a newer -std should do it. I think the GCC Travis has is a bit older and doesn't have static_asserts by default.

@fitzgen
Copy link
Member Author

fitzgen commented Mar 9, 2017

Instead of that, I think compiling with a newer -std should do it. I think the GCC Travis has is a bit older and doesn't have static_asserts by default.

I tried that, and travis's gcc complained that it didn't understand -std and refused to compile :-/

emilio
emilio approved these changes Mar 9, 2017
View reviewed changes
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.

r=me, this is awesome, thanks for bringing them back, and for the tests! :)

src/codegen/mod.rs
4 => quote_ty!(ctx.ext_cx(), u32),
2 => quote_ty!(ctx.ext_cx(), u16),
1 => quote_ty!(ctx.ext_cx(), u8),
_ => panic!("physical field containing bitfields should be sized \
Copy link
Contributor

Choose a reason for hiding this comment

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

well, I guess someone could make bindgen panic with something like: __u128 foo: 128 or something? If so, maybe we just want to skip the getter/setter?

(Not a big deal landing as-is for now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good catch, I didn't think of that. I'll make an E-Easy issue to fix it.

src/codegen/mod.rs
pub fn $getter_name(&self) -> $bitfield_ty {
let mask = $mask as $field_int_ty;
let field_val: $field_int_ty = unsafe {
::$prefix::mem::transmute(self.$field_ident)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice way to not having to care about the alignment ;)

@fitzgen
Copy link
Member Author

fitzgen commented Mar 9, 2017

Thanks for the review! :) @bors-servo r=emilio

@bors-servo
Copy link

📌 Commit 50ee737 has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 50ee737 with merge fdea868...

bors-servo pushed a commit that referenced this pull request Mar 9, 2017
Auto merge of #567 - fitzgen:bitfield-accessors, r=emilio
fdea868 
Reintroduce bitfield accessors

This commit reintroduces accessor methods for bitfields in the generated
bindings.

Fixes #519

r? @emilio
@bors-servo
Copy link

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

@bors-servo bors-servo merged commit 50ee737 into rust-lang:master Mar 9, 2017
@fitzgen fitzgen deleted the bitfield-accessors branch March 9, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fitzgen @highfive @emilio @bors-servo