Generate bitfield accessor names eagerly #1058

pepyakin · 2017-10-04T15:33:51Z

I'm not sure about deanonymize_fields, as we now assign names to data members and also generate names for the bitfield accessors, but can't come up with a better name.

highfive · 2017-10-04T15:33:54Z

Warning

These commits modify unsafe code. Please review it carefully!

fitzgen

Looks good, but I have a few questions below and a couple nitpicks that I'd like to see addressed before we merge this.

Thanks!

fitzgen · 2017-10-04T16:08:37Z

src/ir/comp.rs

+
+    /// Name of the generated Rust setter for this bitfield.
+    pub fn setter_name(&self) -> &str {
+        self.setter_name.as_ref().unwrap()


Let's replace these unwraps with expects that detail the invariant that is being broken if this panics:

self.setter_name .as_ref() .expect("`Bitfield::setter_name` should only be called after assigning field names")

And the same for getter_name.

Yeah, sure!

fitzgen · 2017-10-04T16:09:34Z

src/ir/comp.rs

@@ -668,30 +690,92 @@ impl CompFields {
        );
    }

-    fn deanonymize_fields(&mut self) {
+    fn deanonymize_fields(&mut self, ctx: &BindgenContext, methods: &[Method]) {


Maybe assign_field_names is a slightly better name for this method now? I'm not married to either name...

fitzgen · 2017-10-04T16:10:45Z

src/ir/comp.rs

@@ -668,30 +690,92 @@ impl CompFields {
        );
    }

-    fn deanonymize_fields(&mut self) {
+    fn deanonymize_fields(&mut self, ctx: &BindgenContext, methods: &[Method]) {
+        use std::collections::HashMap;


Any particular reason that this isn't at the top level? If not, let's put it there.

fitzgen · 2017-10-04T16:17:11Z

src/ir/context.rs

+    /// 
+    /// Panics if there is no item for the given `FunctionId` or if the resolved
+    /// item is not a `Function`. 
+    pub fn resolve_func(&self, func_id: FunctionId) -> &Function {


fitzgen · 2017-10-04T16:22:27Z

src/ir/context.rs

+                .unwrap()
+                .deanonymize_fields(self);
+
+            self.items.insert(id, item);


Why do we have to remove the item and then put it back again? Why doesn't the previous approach work anymore?

This results in two passes over the items, while the previous approach was a single pass because there was no collect call on the iterator, just the for loop.

I doubt it is a ton of overhead, and the code isn't so difficult to follow that I'm worried about code paths where we forget to return the item into the set, but I guess I'm just a little confused and it seems like the old approach was simpler.

Yeah, old approach was definitely simpler! I chose this approach because Context was not needed, but now it is. And because of this borrow checker become sad (because we are borrowing self.items). I tried to temporary mem::replace whole self.items but it didn't work out, because we actually need items array (see call to resolve_function in has_method).

So I decided to go approach like in compute_bitfield_units.
I thought that I should factor out this pattern out, but I completely forgot about it 😳

bors-servo · 2017-10-04T21:42:03Z

☔ The latest upstream changes (presumably #1059) made this pull request unmergeable. Please resolve the merge conflicts.

Also clean a bit.

Also make impl_partialeq test to also cover impl_debug case.

fitzgen

👍

Couple nitpicks -- sorry if I missed them in an earlier revision! Will r+ ASAP when they're addressed!

fitzgen · 2017-10-05T16:59:05Z

src/ir/comp.rs

+
+    /// Get the name of this bitfield.
+    fn name(&self) -> Option<&str> {
+        self.data.name()


This is identical to <Bitfield as FieldMethods>::name right above -- I don't think it is needed. Am I missing something?

fitzgen · 2017-10-05T16:59:27Z

src/ir/comp.rs

+    /// Name of the generated Rust setter for this bitfield.
+    /// 
+    /// Panics if called before assigning bitfield accessor names or if 
+    /// this bitfield have no name.


Thanks for these comments :)

fitzgen · 2017-10-05T17:13:38Z

src/ir/context.rs

+    ///
+    /// Panics if attempt to resolve given `ItemId` inside the given
+    /// closure is made.
+    fn with_loaned_item<F: FnOnce(&BindgenContext, &mut Item)>(


For legibility, let's move the FnOnce bound to a where clause. Additionally, we should allow the function to return things.

fn with_loaned_item<F, T>(&mut self, id: ItemId, f: F) -> T where F: FnOnce(&BindgenContext, &mut Item) -> T { ... }

Yeah, I agree, it is more legible. But rustfmt thinks otherwise.
Is rustfmt used to move bounds into where in the past?

Additionally, we should allow the function to return things.

Good point!

fitzgen · 2017-10-05T17:14:56Z

src/ir/context.rs

+            .collect();
+
+        for id in comp_item_ids {
+            self.with_loaned_item(id, |ctx, item| {


This API is pretty nice to use :)

fitzgen

Thanks!!

fitzgen · 2017-10-05T17:54:36Z

@bors-servo r+

bors-servo · 2017-10-05T17:54:37Z

📌 Commit 91796cf has been approved by fitzgen

@fitzgen

Generate bitfield accessor names eagerly @fitzgen r? I'm not sure about `deanonymize_fields`, as we now assign names to data members and also generate names for the bitfield accessors, but can't come up with a better name.

bors-servo · 2017-10-05T17:54:40Z

⌛ Testing commit 91796cf with merge 7bbfb44...

bors-servo · 2017-10-05T18:40:48Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 7bbfb44 to master...

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

fitzgen reviewed Oct 4, 2017

View reviewed changes

highfive added the S-needs-rebase label Oct 4, 2017

Eagerly generate bitfield accessor names

c4c4ae2

Also clean a bit.

pepyakin force-pushed the bitfield-accessors branch from b708e07 to 8db2d66 Compare October 5, 2017 10:45

pepyakin removed the S-needs-rebase label Oct 5, 2017

Use bitfield getter_name in impl_partialeq

af967d7

pepyakin force-pushed the bitfield-accessors branch from 8db2d66 to 9827ad5 Compare October 5, 2017 14:49

Use bitfield getter_name in impl_debug.

a1c6802

Also make impl_partialeq test to also cover impl_debug case.

pepyakin force-pushed the bitfield-accessors branch from 9827ad5 to 3ff8dba Compare October 5, 2017 15:04

fitzgen reviewed Oct 5, 2017

View reviewed changes

pepyakin added 3 commits October 5, 2017 20:43

Introduce with_loaned_item.

e14a49a

rustfmt

83cfe89

Review fixes

91796cf

pepyakin force-pushed the bitfield-accessors branch from 3ff8dba to 91796cf Compare October 5, 2017 17:45

fitzgen approved these changes Oct 5, 2017

View reviewed changes

highfive assigned fitzgen Oct 5, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 5, 2017

bors-servo merged commit 91796cf into rust-lang:master Oct 5, 2017

highfive removed the S-awaiting-merge label Oct 5, 2017

pepyakin deleted the bitfield-accessors branch October 5, 2017 18:43

Generate bitfield accessor names eagerly #1058

Generate bitfield accessor names eagerly #1058

Uh oh!

Conversation

pepyakin commented Oct 4, 2017

Uh oh!

highfive commented Oct 4, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Oct 4, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Oct 5, 2017

Uh oh!

bors-servo commented Oct 5, 2017

Uh oh!

bors-servo commented Oct 5, 2017

Uh oh!

bors-servo commented Oct 5, 2017

Uh oh!

Uh oh!