First steps to fix issue #57 #282

impowski · 2016-11-18T15:55:44Z

This should generate tests for fully specialized templates.
TODO:

Tests

highfive · 2016-11-18T15:55:46Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

emilio

This looks good! A few comments

emilio · 2016-11-18T18:10:08Z

libbindgen/src/codegen/mod.rs

@@ -656,8 +656,30 @@ impl CodeGenerator for CompInfo {
        //
        // TODO: Generate layout tests for template specializations, yay!
        if self.has_non_type_template_params() ||


You should still bail out if has_non_type_template_params, I don't think a lot of stuff will happen, since they probably don't have layout, but still seems a bit clearer.

emilio · 2016-11-18T18:11:12Z

libbindgen/src/codegen/mod.rs

+                let canonical_name = item.canonical_name(ctx);
+
+                if let Some(layout) = layout {
+                    let fn_name = format!("bindgen_test_layout_template_{}", canonical_name);


As discussed before, this will probably need to include the type id of the specialization, or similar, so it generates a totally unique name.

emilio · 2016-11-18T18:11:43Z

libbindgen/src/codegen/mod.rs

+                                                    ::$prefix::mem::align_of::<$ident>());
+                    let size = layout.size;
+                    let align = layout.align;
+                    let item = quote_item!(ctx.ext_cx(),


Probably we should try to reuse this code with the other tests, that way it's easier to change in the future if needed.

emilio

The rest looks neat! I'd really like to move the generation of the function to the test function, but looks great otherwise :)

emilio · 2016-11-21T13:09:06Z

libbindgen/src/codegen/mod.rs

@@ -655,9 +655,43 @@ impl CodeGenerator for CompInfo {
        // also don't output template specializations, neither total or partial.
        //
        // TODO: Generate layout tests for template specializations, yay!


nit: Remove this TODO.

emilio · 2016-11-21T13:09:15Z

libbindgen/src/codegen/mod.rs

@@ -655,9 +655,43 @@ impl CodeGenerator for CompInfo {
        // also don't output template specializations, neither total or partial.
        //
        // TODO: Generate layout tests for template specializations, yay!
+


nit: And this newline.

emilio · 2016-11-21T13:10:38Z

libbindgen/src/codegen/mod.rs

+                        }
+                    }
+
+                    let fn_name = format!("bindgen_test_layout_template_{}{}", canonical_name, types);


This is ok probably, but we could just have done something like:

let fn_name = format!("__bindgen_test_layout_template_{}", item.id().as_usize());

This way is prettier, though a bit more likely to cause conflicts.

emilio · 2016-11-21T13:12:35Z

libbindgen/src/codegen/mod.rs

@@ -655,9 +655,43 @@ impl CodeGenerator for CompInfo {
        // also don't output template specializations, neither total or partial.
        //
        // TODO: Generate layout tests for template specializations, yay!
+
+        // TODO (imp): I will keep it like that right now and move it to function later
        if self.has_non_type_template_params() ||


Please divide this condition as discussed earlier:

if self.has_non_type_template_params() { return; } if self.is_template_specialization() { // ... return; }

impowski · 2016-11-21T17:05:28Z

I applied changes from your review. If it's fine I guess I will rebase it and can work on something else in this project.
r? @emilio

emilio · 2016-11-21T21:18:43Z

Looks good, thanks!

@bors-servo: r+

bors-servo · 2016-11-21T21:18:44Z

📌 Commit 5c76483 has been approved by emilio

bors-servo · 2016-11-21T21:18:47Z

⌛ Testing commit 5c76483 with merge ec9852a...

@emilio

First steps to fix issue #57 This should generate tests for fully specialized templates. TODO: - [x] Tests r? @emilio

bors-servo · 2016-11-21T21:28:45Z

☀️ Test successful - status-travis

Fixes rust-lang#282

Related rust-lang#282

highfive assigned emilio Nov 18, 2016

highfive added the S-awaiting-review label Nov 18, 2016

emilio reviewed Nov 18, 2016

View reviewed changes

impowski added 3 commits November 20, 2016 11:52

First steps to fix issue #57

515ef38

Take out template arguments and make unique names

bed241e

Add new expectations for tests

5ee3601

emilio reviewed Nov 21, 2016

View reviewed changes

Apply fixes due code review for issue #57

5c76483

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 21, 2016

bors-servo pushed a commit that referenced this pull request Nov 21, 2016

Auto merge of #282 - impowski:layout_template_specializations, r=emilio

ec9852a

First steps to fix issue #57 This should generate tests for fully specialized templates. TODO: - [x] Tests r? @emilio

bors-servo merged commit 5c76483 into rust-lang:master Nov 21, 2016

highfive removed the S-awaiting-merge label Nov 21, 2016

impowski deleted the layout_template_specializations branch November 22, 2016 07:50

impowski restored the layout_template_specializations branch November 22, 2016 07:55

impowski deleted the layout_template_specializations branch November 22, 2016 07:55

luser pushed a commit to luser/rust-bindgen that referenced this pull request Mar 27, 2017

Add options to enable derive_debug, disabled by default.

08a32d5

Fixes rust-lang#282

luser pushed a commit to luser/rust-bindgen that referenced this pull request Mar 27, 2017

derive(Debug) on unions.

af4ab06

Related rust-lang#282

luser pushed a commit to luser/rust-bindgen that referenced this pull request Mar 27, 2017

derive(Debug) on cenums.

d0a9bde

Related rust-lang#282

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

First steps to fix issue #57 #282

First steps to fix issue #57 #282

Uh oh!

impowski commented Nov 18, 2016 •

edited

Loading

Uh oh!

highfive commented Nov 18, 2016

Uh oh!

emilio left a comment

Uh oh!

emilio Nov 18, 2016

Uh oh!

emilio Nov 18, 2016

Uh oh!

emilio Nov 18, 2016

Uh oh!

emilio left a comment

Uh oh!

emilio Nov 21, 2016

Uh oh!

emilio Nov 21, 2016

Uh oh!

emilio Nov 21, 2016

Uh oh!

emilio Nov 21, 2016

Uh oh!

impowski commented Nov 21, 2016 •

edited

Loading

Uh oh!

emilio commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

Uh oh!

First steps to fix issue #57 #282

First steps to fix issue #57 #282

Uh oh!

Conversation

impowski commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Nov 18, 2016

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

emilio Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

impowski commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

Uh oh!

impowski commented Nov 18, 2016 •

edited

Loading

impowski commented Nov 21, 2016 •

edited

Loading