Generate member functions #36

weiznich · 2017-08-03T13:30:34Z

This pull request tries to make the generated c++ header more idiomatic.
In detail it tries to generate small member functions wrapping the exported functions for this type.

For example for the following rust code

#[repr(C)]
struct MyStruct {
    some_value: i32,
}


#[no_mangle]
extern "C" fn do_something(a: *const MyStruct) {}

The following c++ header is generated

struct MyStruct;

void do_something(MyStruct* a);

struct MyStruct {
    uint32_t some_value;

    void do_something() {
        return ::do_something(this);
    }
};

In addition to generate more idiomatic code this allows to enforce some invariants of the underlying rust code. For example one could now safely assume that the pointer a for the function do_something is not null.

Future improvements for this feature may be to generate also constructor and destructor wrappers.
I'm not sure about the syntax of this yet.

eqrion

Did a quick pass through, I'd like to take another look after the comments have been addressed.

eqrion · 2017-08-07T19:21:48Z

src/bindgen/library.rs

@@ -633,8 +634,10 @@ impl Library {
        // Gather only the items that we need for this
        // `extern "c"` interface
        let mut deps = DependencyList::new();
+        let mut oop = MemberFunctions::new();


What does oop stand for?

Object orientated programming (It is a bad name, should better be something like member_functions)

eqrion · 2017-08-07T19:22:34Z

src/bindgen/ir/structure.rs

+    out.write("return ::");
+    out.write(&f.name);
+    out.write("(");
+    let align_lenght = out.line_length_for_align();


'align_length'

eqrion · 2017-08-07T19:23:26Z

src/bindgen/library.rs

@@ -873,6 +914,16 @@ impl GeneratedBindings {
        }

        if self.config.language == Language::Cxx {
+            out.new_line();


Why two new lines?

To separate those implementations visually from the functions before.

One new line is needed to start a new line after a function definition, the second is inserted to have an empty line between functions and classes.

eqrion · 2017-08-07T19:25:52Z

src/bindgen/ir/function.rs

+    pub fn add_member_function(&self, out: &mut MemberFunctions) {
+        if let Some(&(_, ref ty)) = self.args.get(0) {
+            match *ty {
+                Type::ConstPtr(ref t) | Type::Ptr(ref t) => {


We need to make these member functions const correct.

fn do_something(foo: *const Foo)

Should generate

struct Foo { void do_something() const { ::do_something(this) } }

eqrion · 2017-08-07T19:28:51Z

src/bindgen/ir/structure.rs

@@ -230,6 +255,33 @@ impl Source for Struct {
    }
 }

+fn format_function_call_1<W: Write>(f: &Function, out: &mut SourceWriter<W>) {
+    out.write("return ::");


It'd be nice to omit the return if the return type is void, but from my brief testing it isn't a compile warning/error.

eqrion · 2017-08-07T19:30:07Z

src/bindgen/library.rs

+                                documentation: s.documentation.clone(),
+                            };
+                            result.items.push(PathValue::OpaqueItem(opaque));
+                            s.functions = functions.into_iter()


Can you add a comment mentioning essentially that you're stripping out the this argument?

eqrion · 2017-08-07T19:31:01Z

src/bindgen/library.rs

@@ -758,6 +797,7 @@ pub struct GeneratedBindings {
    monomorphs: Monomorphs,
    items: Vec<PathValue>,
    functions: Vec<Function>,
+    full_objects: Vec<Struct>,


Can you rename this to member_function_structs? I don't mind the longer name as long as it is clear.

eqrion · 2017-08-07T19:54:50Z

src/bindgen/ir/structure.rs

@@ -24,6 +24,7 @@ pub struct Struct {
    pub generic_params: Vec<String>,
    pub documentation: Documentation,
    pub functions: Vec<Function>,
+    pub destructor: Option<Function>,


I'm worried this could be a potential footgun. In most cases if you implement any of the copy constructor, copy assignment, or destructor, you need to implement them all. See the rule of three.

For example

struct WrapperVec { data: Vec<u8>, } fn destructor(this: *WrapperVec) { mem::drop(*this) }

{ WrapperVec a = ... WrapperVec b = a; // default copy constuctor does a shallow copy of the vec ptr's } // destructor for a and b runs and the vec is dropped twice leading to a double free

This is a tricky problem. If we were to disable the copy constructor and copy assignment and only allow moves, I think we'd be in a better position. That would disadvantage rust structs that truly are POD. We could maybe generate clone() methods then?

I think whatever the solution is, it needs to be complete so we can't just generate destructors.

Seems like my c++ got a bit rusty 😄
What about the following idea:
If there is a destructor the exported struct is likely no POD and does not implement Copy on rust side. Therefore the copy constructor and copy assignment operator should be disabled. ( A future version may allow them by defining a clone function(like the destructor) that is internally called)
For me this seems like the solution that matches the underlying behavior of the rust code.

I think whatever the solution is, it needs to be complete so we can't just generate destructor.

I've pushed the current implementation to see what others think about the idea. I've developed this while trying to generate a usable api for our internal project. Internally all my functions to free some allocated memory are implemented to reset the struct in a non allocating state, meaning free all memory and then set all pointers to null pointers. So the copy constructor should not harm this implementation. (But this is defined by my implementation, not by the generated header, so this is a not so great solution)

Add a option to generate member function wrappers for each function taking a pointer to a struct type as first argument

If a destructor is implemented it is unsafe to have a copy constructor without having a clone implementation (Future versions may allow to have a copy construtor if a clone method is provieded from rust side)

weiznich · 2017-08-11T09:07:36Z

I've fixed all comments.

eqrion · 2017-08-12T08:05:42Z

I'm sorry about the inconvenience, but this will probably need a rebase with the changes that have just landed.

weiznich · 2017-08-16T13:05:17Z

I'm sorry about the inconvenience, but this will probably need a rebase with the changes that have just landed.

As written in #42 I think it is not possible to implement this with the current dependency management in cbindgen. So we will need to improve this first.

Also I've found some additional problems related to the c++(gcc, clang) calling conventions.
For example if I have the following rust code:

#[repr(c)]
struct Foo {
    a: i32
}

#[no_mangle]
pub extern "C" fn make_foo(a: i32) -> Foo {
    Foo {a }
}

#[no_mangle]
pub extern "C" fn some_member(foo: *mut Foo) {}

The straight forward C++ header will look like this:

struct Foo;

extern "C" void some_member(Foo* foo);

extern "C" struct Foo {
    int32_t a;
    
    void some_member() {
        ::some_member(this);
    }
};

extern "C" Foo make_foo(int32_t a);

When you try to use this code from some c++ programm, you will notice that something strange happens when you call make_foo. (The value passed will not be equal the value received on rust side). Gcc happily accepts this code 😥 while clang emitts the following warning:

'make_foo' has C-linkage specified, but returns user-defined type 'Foo' which is incompatible with C

To solve this we need to generate some more wrapping code:

struct Foo;

extern "C" void some_member(Foo* foo);

namespace intern {
    extern "C" struct Foo_intern {
        int32_t a;
    };
}
struct Foo: public intern::Foo_intern {
    Foo(intern::Foo_intern&& other) {
        this->a = other.a;
    }

    void some_member() {
        ::some_member(this);
    }
};

namespace intern {
    extern "C" Foo_intern make_foo(int32_t a);
}

inline Foo make_foo(int32_t a) {
    return intern::make_foo(a);
}

See this commit for an implementation.

The big question now is how to move forward on this?
Do you want such a feature in cbindgen?
If yes how do we want to merge back all those changes? (If not, I'm also fine continue to use my own fork)

eqrion · 2017-08-16T16:23:27Z

The big question now is how to move forward on this?
Do you want such a feature in cbindgen?
If yes how do we want to merge back all those changes? (If not, I'm also fine continue to use my own fork)

So in the future I would like a feature like this. My goal with cbindgen is to make using a rust library from C/C++ safer and more ergonomic (in that order). Right now I don't have a whole lot of time for the library so I've been focusing on smaller things. This feature seems like it'll require a lot of careful thought and will be a significant investment for the codebase so I'm hesitant to merge anything right now.

I appreciate the work you've been doing, and would like to see what comes of it.

eqrion · 2017-11-09T05:13:43Z

As there isn't a clear answer to these C++ and Rust interop/safety questions, I'm going to close this for now.

weiznich · 2017-11-13T10:52:24Z

As there isn't a clear answer to these C++ and Rust interop/safety questions, I'm going to close this for now.

As far as I remember all raised questions seems to be solved in my fork. The "biggest" issue was that thing with the calling conventions, that can be solved by generating some wrapper code as shown in this comment.

eqrion reviewed Aug 7, 2017

View reviewed changes

weiznich added 7 commits August 10, 2017 20:10

For generated c++ headers generate small member wrapper functions

7316d02

Add a option to generate member function wrappers for each function taking a pointer to a struct type as first argument

Remove some unneeded checks for generating documentation

a577a6e

Support destructors

c6ad53c

Explicitly delete copy constructor and copy assigne

b065487

If a destructor is implemented it is unsafe to have a copy constructor without having a clone implementation (Future versions may allow to have a copy construtor if a clone method is provieded from rust side)

Skip outputing a return statement if a function only returns void

3b58687

Fix constness of generated member functions

0e9607e

Address review comments

506297f

weiznich force-pushed the feature/member_functions branch from b729aff to 506297f Compare August 11, 2017 09:05

weiznich mentioned this pull request Aug 14, 2017

Fix order of structs #42

Closed

eqrion closed this Nov 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate member functions #36

Generate member functions #36

weiznich commented Aug 3, 2017

eqrion left a comment

eqrion Aug 7, 2017

weiznich Aug 7, 2017

eqrion Aug 7, 2017

eqrion Aug 7, 2017

weiznich Aug 7, 2017

eqrion Aug 7, 2017

eqrion Aug 7, 2017

eqrion Aug 7, 2017

eqrion Aug 7, 2017

eqrion Aug 7, 2017

weiznich Aug 7, 2017

weiznich commented Aug 11, 2017

eqrion commented Aug 12, 2017

weiznich commented Aug 16, 2017

eqrion commented Aug 16, 2017

eqrion commented Nov 9, 2017

weiznich commented Nov 13, 2017

Generate member functions #36

Generate member functions #36

Conversation

weiznich commented Aug 3, 2017

eqrion 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

weiznich commented Aug 11, 2017

eqrion commented Aug 12, 2017

weiznich commented Aug 16, 2017

eqrion commented Aug 16, 2017

eqrion commented Nov 9, 2017

weiznich commented Nov 13, 2017