Added mdbook entry for bitfields. #1052

ambaxter · 2017-10-01T16:15:43Z

Fixes #818

highfive · 2017-10-01T16:15:46Z

Warning

These commits modify unsafe code. Please review it carefully!

fitzgen

This is a good start! Thanks @ambaxter.

I think we should have a section at the very beginning that gives an overview of bindgen's translation strategy:

The Rust language does not have bitfields, so they are translated into opaque fields in the Rust struct
We generate getter methods to access the bitfields, and the getters are named <bitfield>
We generate setter methods to write to bitfields, and the setters are named set_<bitfield>
We generate constructors for the opaque fields containing a set of bitfields. These constructors are static methods on the Rust struct, and are named new_bitfield_{1,2,...}. They have a parameter for each of the bitfields contained within the opaque physical field.

With that leading overview section in place, I think the existing detailed walkthrough will be a good follow up for people who want more.

Thanks again! Looking forward to the revised version :)

fitzgen · 2017-10-02T16:11:46Z

book/src/using-bitfields.md

+bitfield create_bitfield();
+
+// Print a bitfield
+void print_bitfield(bitfield bfield)


Nitpick: missing a semicolon at the end of this declaration.

fitzgen · 2017-10-02T16:18:35Z

book/src/using-bitfields.md

+bitfield: a:0, b:0, c:0
+```
+
+However, Bindgen's getter provides different results: 


This is a bug, that we should fix, not behavior that we should document and canonicalize.

Let's remove this section and file a new issue to fix this bug instead.

fitzgen · 2017-10-02T16:20:09Z

book/src/using-bitfields.md

+    unsigned int b: 1;
+    unsigned int c: 2;
+
+} bitfield;


I think naming the struct that contains bitfields "bitfield" leaves room for misinterpretation. The struct is not a bitfield, it just contains bitfields. How about ContainsBitfields or StructWithBitfields?

fitzgen · 2017-10-02T16:33:35Z

book/src/using-bitfields.md

+bitfield: a:1, b:1, c:0
+```
+
+To create a new bitfield in Rust, use mem::zeroed.


Rather than use std::mem::zeroed, we should use the bitfield allocation unit constructors that bindgen emits: new_bitfield_{1,2,...}. About one of these is created for each set of contiguous bitfields within a struct.

let contains_bitfields = ContainsBitfields { _bitfield_1: ContainsBitfields::new_bitfield_1(/* a */ 1, /* b */ 2, /* c */ 3), ..Default::default() };

@fitgen with the currently generated code, e.g. this, the bitfield allocation unit constructor does not necessarily return a value of the same type as the bitfield allocation unit, so this way of initialising a field produces a compile error. What should the user do in this case?

For now transmute, but really we should (eventually) fix that.

The problem is transmute is not a const fn yet, and we make these bitfield constructors const fns when we can. Additionally, making transmute into a const fn requires Miri integration into rustc, and I'm not sure on the timeline for that.

We could do all kinds of things to work around this like:

avoid generating the ctors when we need to transmute and its a const fn

avoid making the ctor a const fn when we need to transmute

skip generating any ctor at all when we need to transmute

etc

@fitzgen Is a fix along these lines reasonable?

@fitzgen During testing, I ran into an issue where Default isn't implemented for the bitfield. Is there a flag I need to set for that to be generated?

fitzgen · 2017-10-02T16:56:25Z

book/src/using-bitfields.md

+bitfield: a:1, b:1, c:3
+```
+
+Overflowing a bitfield will result in the same behavior as in C/C++: the bitfield will be set to 0.


Does it just happen to set to 0 in this case or is that really what the standard says should happen? My reading of the C++ standard doesn't come up with anything either way. Same with the C11 standard.

I think we should leave overflow unspecified as well.

In my googling Stackoverflow that's what the standard says: https://stackoverflow.com/questions/4908300/overflow-in-bit-fields#4908606

fitzgen · 2017-10-12T20:57:10Z

Hey @ambaxter, still making progress here?

ambaxter · 2017-10-12T21:18:02Z

Yes. Work took a lot of my free time the last few weeks. Thankfully today is the last day of that.

ambaxter · 2017-11-16T13:30:32Z

As an update, the latest crunch time is over. Combining that with the upcoming US holiday, I should have this updated this week.

ambaxter · 2017-11-26T21:37:54Z

Ready for next review

fitzgen · 2017-11-27T18:06:53Z

@ambaxter this PR is a bit tough to review in its current state: there seems to be lots of unrelated commits from master mixed in (perhaps a funky merge?)

Can you rebase just your commits on top of master and squash them down to one commit? Alternatively, just check out the latest master and copy your new .md file into it and make a new commit and force push that to this PR.

Thanks!

fitzgen · 2017-11-27T18:07:16Z

And thanks for picking this up again!

ambaxter · 2017-11-27T18:37:24Z

Weird. Those were from the rebase. I'll reset it.

fitzgen

Looks great! A couple tiny nit picks to be addressed below and then we can finally merge this. Thanks for sticking with this PR @ambaxter!

fitzgen · 2017-11-29T18:34:58Z

book/src/using-bitfields.md

+
+## Bitfield Strategy Overview
+
+As Rust does not support bitfields, Bindgen generates an opaque struct for each with the following characteristics


generates an opaque struct

Nitpick: We don't make the whole struct opaque, we emit an opaque physical field that contains one or more logical bitfields.

fitzgen · 2017-11-29T18:36:17Z

book/src/using-bitfields.md

+As Rust does not support bitfields, Bindgen generates an opaque struct for each with the following characteristics
+* Immutable getter functions for each bitfield named ```<bitfield>```
+* Setter functions for each bitfield named ```set_<bitfield>```
+* A static constructor  ```new_bitfield_{1, 2, ...}``` with a parameter for each bitfield contained within the opaque physical field.


Nitpick: s/for each bitfield/for each contiguous block of bitfields/

There is a constructor for each of those physical, opaque fields that we end up generating, which is roughly equivalent to sets of contiguous bitfields.

fitzgen · 2017-11-29T18:36:58Z

book/src/using-bitfields.md

+    let bfield = StructWithBitfields{
+        _bitfield_1: StructWithBitfields::new_bitfield_1(0,0,0),
+        ..Default::default()
+    };    unsafe { print_bitfield(bfield) };


Nitpick: need to insert a new line before the unsafe block.

fitzgen · 2017-11-29T18:37:15Z

book/src/using-bitfields.md

+* Setter functions for each bitfield named ```set_<bitfield>```
+* A static constructor  ```new_bitfield_{1, 2, ...}``` with a parameter for each bitfield contained within the opaque physical field.
+
+## Bitfield examples


awesome 👍

ambaxter · 2018-01-16T15:51:51Z

Finally got some time to finish this.

fitzgen

Awesome! Thanks so much for circling back on this @ambaxter! Just one typo fix needed, then we can merge this.

fitzgen · 2018-01-17T17:57:07Z

book/src/using-bitfields.md

+
+As Rust does not support bitfields, Bindgen generates a struct for each with the following characteristics
+* Immutable getter functions for each bitfield named ```<bitfield>```
+* Setter functions for each contiguous bsock of bitfields named ```set_<bitfield>```


"bsock" -> "block"

fitzgen · 2018-01-17T20:34:21Z

Thanks!

@bors-servo r+

bors-servo · 2018-01-17T20:34:22Z

📌 Commit b969bfb has been approved by fitzgen

bors-servo · 2018-01-17T20:34:25Z

⌛ Testing commit b969bfb with merge 13f2b9a...

Added mdbook entry for bitfields. Fixes #818

bors-servo · 2018-01-17T20:51:14Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 13f2b9a to master...

highfive added the S-awaiting-review label Oct 1, 2017

ambaxter force-pushed the bitfield_docs branch from 7fc2e6e to bd644d2 Compare October 2, 2017 11:14

fitzgen requested changes Oct 2, 2017

View reviewed changes

Squashed commit.

d4ffe9e

ambaxter force-pushed the bitfield_docs branch from 9deaf5c to d4ffe9e Compare November 27, 2017 18:43

fitzgen reviewed Nov 29, 2017

View reviewed changes

Updated to fix nitpicks

7b34d5c

fitzgen approved these changes Jan 17, 2018

View reviewed changes

Fixed typo.

b969bfb

highfive assigned fitzgen Jan 17, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 17, 2018

bors-servo pushed a commit that referenced this pull request Jan 17, 2018

Auto merge of #1052 - ambaxter:bitfield_docs, r=fitzgen

13f2b9a

Added mdbook entry for bitfields. Fixes #818

bors-servo merged commit b969bfb into rust-lang:master Jan 17, 2018

highfive removed the S-awaiting-merge label Jan 17, 2018

ambaxter deleted the bitfield_docs branch January 17, 2018 21:45


		## Bitfield Strategy Overview

		As Rust does not support bitfields, Bindgen generates an opaque struct for each with the following characteristics

Added mdbook entry for bitfields. #1052

Added mdbook entry for bitfields. #1052

Conversation

ambaxter commented Oct 1, 2017 • edited by fitzgen Loading

highfive commented Oct 1, 2017

fitzgen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyn Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Oct 12, 2017

ambaxter commented Oct 12, 2017

ambaxter commented Nov 16, 2017

ambaxter commented Nov 26, 2017

fitzgen commented Nov 27, 2017

fitzgen commented Nov 27, 2017

ambaxter commented Nov 27, 2017

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambaxter commented Jan 16, 2018

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Jan 17, 2018

bors-servo commented Jan 17, 2018

bors-servo commented Jan 17, 2018

bors-servo commented Jan 17, 2018

ambaxter commented Oct 1, 2017 •

edited by fitzgen

Loading

fitzgen left a comment •

edited

Loading

glyn Nov 13, 2017 •

edited

Loading