Add support for constructors, and integration tests. #332

emilio · 2016-12-13T13:33:22Z

r? @fitzgen

cc @vvuk

highfive · 2016-12-13T13:33:26Z

Warning

These commits modify unsafe code. Please review it carefully!

…tmp, to avoid potential conflicts.

vvuk · 2016-12-13T16:29:58Z

Works great with Skia, fwiw. Thanks!

I'll file a feature request -- would be nice to be able to rename the rust-side constructors based on their signature by passing in some kind of mapping file to bindgen, otherwise there's no guarantee that the function names will be stable (e.g. if upstream adds another constructor/overload).

fitzgen

LGTM! Just a couple comments that I am not sure require any code changes, just clarification for my benefit.

r=me when you're ready to land :)

Thanks!

fitzgen · 2016-12-13T16:20:22Z

libbindgen/tests/headers/constructors.hpp

@@ -0,0 +1,13 @@
+
+class TestOverload {
+  // This one shouldnt' be generated.


Because it is private, right?

fitzgen · 2016-12-13T16:24:46Z

libbindgen/tests/headers/constructors.hpp

+  TestOverload();
+public:
+  TestOverload(int);
+  TestOverload(double);


Do we handle when the class already has a new method properly? Do we just treat the constructors or the new methods as overloads? Care to add a small test case?

Oh wait -- new is a keyword ofc -- nvm

fitzgen · 2016-12-13T16:31:05Z

libbindgen/src/ir/function.rs

@@ -147,6 +147,7 @@ impl FunctionSig {
        };
        let mut args: Vec<_> = match cursor.kind() {
            CXCursor_FunctionDecl |
+            CXCursor_Constructor |
            CXCursor_CXXMethod => {


Is there a reason this doesn't also match CXCursor_Destructor like the ClangSubItemParser impl further down does?

Is it handled elsewhere?

Ah -- are we not binding destructors yet? That's surprising I really thought we were.

Yeah, we're not doing it. We can without too much trouble though.

When we do, there's no point in doing that, though (we could, I guess?). But destructors have no arguments.

fitzgen · 2016-12-13T16:31:34Z

libbindgen/src/ir/function.rs

-            let is_static = cursor.method_is_static();
+        let is_method = cursor.kind() == CXCursor_CXXMethod;
+
+        if is_method || cursor.kind() == CXCursor_Constructor {


Ditto about dtors for this check.

Yeah, when we support them that check should be there. But we should also check for virtual destructors, etc.

fitzgen · 2016-12-13T16:36:17Z

libbindgen/src/codegen/mod.rs

@@ -1359,13 +1359,14 @@ impl MethodCodegen for Method {
        let mut stmts = vec![];

        // If it's a constructor, we need to insert an extra parameter with a
-        // variable called `tmp` we're going to create.
+        // variable called `__bindgen_tmp` we're going to create.


fitzgen · 2016-12-13T16:37:27Z

bindgen-integration/cpp/Test.h

@@ -1,6 +1,6 @@
 #pragma once

-class Test final {


Why no more final? We can't get at it with llvm_stable?

The travis gcc was complaining, and I didn't want to fight that battle :)

fitzgen · 2016-12-13T16:38:22Z

.travis.yml

@@ -38,7 +38,8 @@ script:
  - cargo test --features "$BINDGEN_FEATURES"
  - cargo test --release --features "$BINDGEN_FEATURES"
  - cd ../bindgen-integration
-  - cargo test
+  - cargo test --features "$BINDGEN_FEATURES"
+  - cargo test --release --features "$BINDGEN_FEATURES"


I think this is getting big enough that we should pull it out into a ./ci/test script. In a follow up, of course.

fitzgen · 2016-12-13T16:39:28Z

libbindgen/src/ir/comp.rs

@@ -466,6 +466,7 @@ impl CompInfo {
        &self.methods
    }

+    /// Get this type's set of constructors.


emilio · 2016-12-13T16:57:03Z

@bors-servo r=fitzgen

bors-servo · 2016-12-13T16:57:04Z

📌 Commit 1daf989 has been approved by fitzgen

bors-servo · 2016-12-13T17:08:55Z

⚡ Test exempted - status

@fitzgen

Add support for constructors, and integration tests. r? @fitzgen cc @vvuk

Add support for constructors, and integration tests.

883ff74

highfive assigned fitzgen Dec 13, 2016

highfive added the S-awaiting-review label Dec 13, 2016

emilio added 3 commits December 13, 2016 14:36

codegen: Rename the temporary variable in constructors for __bindgen_…

0f0b6d6

…tmp, to avoid potential conflicts.

Fix travis

bc28da0

ir: Add documentation for CompInfo::constructors

1daf989

fitzgen approved these changes Dec 13, 2016

View reviewed changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 13, 2016

bors-servo merged commit 1daf989 into rust-lang:master Dec 13, 2016

bors-servo pushed a commit that referenced this pull request Dec 13, 2016

Auto merge of #332 - emilio:constructors, r=fitzgen

959ef7c

Add support for constructors, and integration tests. r? @fitzgen cc @vvuk

highfive removed the S-awaiting-merge label Dec 13, 2016

bbigras mentioned this pull request Dec 15, 2016

Constructor support. #250

Closed

Manishearth unassigned fitzgen Jul 25, 2017

		@@ -0,0 +1,13 @@

		class TestOverload {
		// This one shouldnt' be generated.

Add support for constructors, and integration tests. #332

Add support for constructors, and integration tests. #332

Uh oh!

Conversation

emilio commented Dec 13, 2016

Uh oh!

highfive commented Dec 13, 2016

Uh oh!

vvuk commented Dec 13, 2016

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!

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!

emilio commented Dec 13, 2016

Uh oh!

bors-servo commented Dec 13, 2016

Uh oh!

bors-servo commented Dec 13, 2016

Uh oh!

Uh oh!