Can derive copy analysis #866

photoszzt · 2017-07-29T07:20:07Z

fitzgen

Looks good as always :)

A couple nitpicks and questions below.

Thanks!

fitzgen · 2017-07-31T16:11:16Z

src/ir/analysis/derive_copy.rs

+    fn constrain(&mut self, id: ItemId) -> ConstrainResult {
+        trace!("constrain: {:?}", id);
+
+        if self.cannot_derive_copy.contains(&id) || self.cannot_derive_copy_in_array.contains(&id) {


Is it always true that we insert into both sets at the same time, if we will ever insert the same item into both sets? If we would insert into one set on a first iteration, and insert into the other set on a second iteration, then this check here would prevent that second insertion from ever occurring.

If this is indeed correct (I need to think about it some more and read the rest of this code) then we should definitely have a comment here for readers to understand this subtlety.

Yes. I do want to make sure they insert into both sets. Otherwise, I don't have a way to make sure the function reaches a fix-point. I can't check that they are in both sets as there're elements only belongs to the second set. From what I see in the trace if it inserts in the first set, it also needs to insert into the second set. I split it for the situation that it doesn't insert in the first set and insert into the second set (things related to Named both explicitly and transitively).

fitzgen · 2017-07-31T16:12:25Z

src/ir/analysis/derive_copy.rs

+            } else {
+                trace!("    we cannot derive Copy for the layout");
+                self.insert(id);
+                self.insert_array(id)


Seems like having an insert_both method could be nice; these two lines are repeated in multiple places.

fitzgen · 2017-07-31T16:18:40Z

src/ir/analysis/derive_copy.rs

+                            also cannot derive Copy");
+                    self.insert_array(id);
+                }
+                if cant_derive_copy_in_array || cant_derive_copy {


Rather than checking for both conditions here and then maybe returning if either cannot derive, let's do the check-and-maybe-return dance after each lookup, so that we aren't continuing to do lookups when we already "know" we can stop considering this item.

let cannot_derive_copy = ...; if cannot derive_copy { ... return self.insert(id); } let cannot_derive_copy_in_array = ...; if cannot_derive_copy_in_array { ... return self.insert_array(id); }

Ah wait, is this so that we insert into both sets at the same time, if need be? (Like how I was asking in an above comment).

If so, then we really need a comment explaining this. It is pretty subtle.

fitzgen · 2017-07-31T16:22:46Z

src/ir/analysis/derive_copy.rs

+                    self.insert_array(id);
+                }
+                if cant_derive_copy_in_array || cant_derive_copy {
+                    return ConstrainResult::Changed;


Same deal here for making sure it goes into both sets before the return?

Yes. See above.

fitzgen · 2017-07-31T16:34:57Z

src/ir/item.rs

-    type Extra = ();
-
-    fn can_derive_copy(&self, ctx: &BindgenContext, _: ()) -> bool {
-        if self.detect_derive_copy_cycle.get() {


We can remove the detect_derive_copy_cycle member now, as well.

fitzgen · 2017-07-31T16:39:02Z

src/ir/template.rs

+    /// Is this has array?
+    fn has_array(&self, ctx: &BindgenContext) -> bool {
+        let resolved_item = ctx.resolve_item(self.template_definition());
+        resolved_item.as_type().map_or(false, |ty| ty.has_array(ctx))


Do we need to consider the template arguments here as well? The old can_derive_copy_in_array for TemplateInstantiation also checked that each of the args could derive copy in an array, but I'm not sure if that translates to HasArray or not.

For HasArray, I mainly checking about whether it's transitively related to a comp that has an array in it. Maybe I should also check args.

Try adding args. I have a feeling that my current implementation runs into cycling recursion and the stack overflows. Maybe it would turn into an analysis.

bors-servo · 2017-07-31T20:02:45Z

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

bors-servo · 2017-07-31T21:44:48Z

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

bors-servo · 2017-08-01T16:56:17Z

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

fitzgen

Just caught something below; sorry I didn't notice this earlier!

Note also that #874 just landed, so this PR will need to rebase on top of its changes.

Thanks @photoszzt !

fitzgen · 2017-08-01T16:13:28Z

src/ir/analysis/has_array.rs

+            }
+
+            TypeKind::Comp(ref info) => {
+                let fields_have = info.fields()


What about base members?

fitzgen · 2017-08-01T17:16:16Z

src/ir/context.rs

+        // derive `Copy` or not.
+        trace!("looking up id: {:?}, contains_array: {:?}", id, contains_array);
+        if contains_array {
+            !self.cannot_derive_copy_in_array.as_ref().unwrap().contains(&id)


Is there ever a case where the type can have an array containing a named type parameter and it is still true that it can derive copy? As far as I know, the answer is "no", which means that this check is already unnecessary. That is, if id.has_array(ctx) then we can return false immediately. Which also means that we don't need the cannot_derive_copy_in_array set, because its redundant with the HasArray analysis's results.

So, unless I'm overlooking something, I think we should take these steps:

Rename HasArrayAnalysis to HasTypeParameterInArray so that it is more clear what it is computing. It isn't computing if the type has any old array in it, it is computing whether it contains an array of type parameters. We can drop the Analysis because it should be obvious from the context: its in the ir::analysis module amd it implements the MonotonFramework trait.

Only compute the cannot_derive_copy set in the CannotDeriveCopy analysis. Get rid of the cannot_derive_copy_in_array set.

In this method:

assert!(... in codegen phase ...); !id.has_type_param_in_array(self) && !self.cannot_derive_copy.as_ref().unwrap().contains(&id)

Sorry for not catching this earlier! I think this will be a big improvement, though, so it is worth it.

fitzgen

Thanks! 👍

fitzgen · 2017-08-03T20:10:02Z

@bors-servo r+

bors-servo · 2017-08-03T20:10:03Z

📌 Commit 89dde21 has been approved by fitzgen

bors-servo · 2017-08-03T20:10:06Z

⌛ Testing commit 89dde21 with merge e91d448...

@fitzgen

Can derive copy analysis r? @fitzgen or @emilio. Fix: #766

bors-servo · 2017-08-03T20:21:44Z

💔 Test failed - status-travis

fitzgen · 2017-08-03T23:04:56Z

@bors-servo r+

bors-servo · 2017-08-03T23:04:57Z

📌 Commit 68f6adf has been approved by fitzgen

@fitzgen

Can derive copy analysis r? @fitzgen or @emilio. Fix: #766

bors-servo · 2017-08-03T23:05:00Z

⌛ Testing commit 68f6adf with merge b1cfa78...

bors-servo · 2017-08-04T01:18:35Z

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

fitzgen approved these changes Jul 31, 2017

View reviewed changes

fitzgen reviewed Aug 1, 2017

View reviewed changes

fitzgen approved these changes Aug 3, 2017

View reviewed changes

bors-servo pushed a commit that referenced this pull request Aug 3, 2017

Auto merge of #866 - photoszzt:can_derive_copy_array, r=fitzgen

e91d448

Can derive copy analysis r? @fitzgen or @emilio. Fix: #766

Can derive copy analysis

68f6adf

bors-servo pushed a commit that referenced this pull request Aug 3, 2017

Auto merge of #866 - photoszzt:can_derive_copy_array, r=fitzgen

b1cfa78

Can derive copy analysis r? @fitzgen or @emilio. Fix: #766

bors-servo merged commit 68f6adf into rust-lang:master Aug 4, 2017

This was referenced Aug 4, 2017

Can derive hash analysis #887

Merged

Feature 832 custom rust target #859

Merged

fitzgen mentioned this pull request Aug 11, 2017

Tools news rust-dev-tools/dev-tools-team#22

Closed

emilio mentioned this pull request Sep 4, 2017

Add bindgen::Builder::derive_copy to control whether we emit derive(Copy) on type definitions #948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Can derive copy analysis #866

Can derive copy analysis #866

photoszzt commented Jul 29, 2017

fitzgen left a comment

fitzgen Jul 31, 2017

photoszzt Jul 31, 2017

fitzgen Jul 31, 2017

fitzgen Jul 31, 2017

fitzgen Jul 31, 2017

fitzgen Jul 31, 2017

photoszzt Jul 31, 2017

fitzgen Jul 31, 2017

fitzgen Jul 31, 2017

photoszzt Jul 31, 2017

photoszzt Jul 31, 2017

bors-servo commented Jul 31, 2017

bors-servo commented Jul 31, 2017

bors-servo commented Aug 1, 2017

fitzgen left a comment

fitzgen Aug 1, 2017

fitzgen Aug 1, 2017

fitzgen left a comment

fitzgen commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

fitzgen commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 4, 2017

Can derive copy analysis #866

Can derive copy analysis #866

Conversation

photoszzt commented Jul 29, 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

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

bors-servo commented Jul 31, 2017

bors-servo commented Jul 31, 2017

bors-servo commented Aug 1, 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

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

fitzgen commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 3, 2017

bors-servo commented Aug 4, 2017