Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: function that returns object by value #39

Open
jcmonnin opened this issue Jan 12, 2017 · 6 comments
Open

Question: function that returns object by value #39

jcmonnin opened this issue Jan 12, 2017 · 6 comments

Comments

@jcmonnin
Copy link
Contributor

Hi,

First, I'm not sure if the issue tracker is the right way to ask questions about the project. I haven't seen a mailing list, so I guess it's ok?

Please consider the example based on 'wrapping.md':

struct X
{
    bool b;
    X(bool b) : b(b) {}
};

static bool test1()
{
    return true;
}

static X test2()
{
    return X(true);
}

void v8ppTestBind(v8::Isolate* isolate)
{
    v8pp::class_<X> X_class(isolate);
    X_class
        .ctor<bool>()
        .set("b", &X::b);
    
    v8pp::module module(isolate);
    module
        .set("X", X_class)
        .set("test1", test1)
        .set("test2", test2);

    isolate->GetCurrentContext()->Global()->Set(v8::String::NewFromUtf8(isolate, "module"), module.new_instance());
}

Executing module.X(true) gives [object X].

Executing module.test1() gives true.

Executing module.test2() gives undefined.

Why is test2 not returning a wrapped object of type X?

@pmed
Copy link
Owner

pmed commented Jan 12, 2017

Hi Jean-Claude,

Sure, it's ok to ask questions here. There is also a gitter chat: https://gitter.im/pmed/v8pp for this project.

The short answer: because test2() returns an unwrapped temporary C++ object.

When your test2() function returns an instance of X class created on C++ side, v8pp converts this X object to a v8::Object handle with an overload of v8pp::to_v8() function that uses class_<X>::find_object(isolate, &instance) function to find an existing wrapped C++ instance of X.

But the result of test2() is a temporary X object which was not registered in v8pp internal structures, and class_<X>::find_object() function returns an empty v8::Object handle, which is then converted to undefined on JavaScript side.

To return a wrapped C++ object available in JavaScript, you have to explicitly state this fact with class_<X>::create_object() function.

There are also class_<X>::import_external(v8::Isolate* isolate, T* ext) and class_<X>::reference_external(v8::Isolate* isolate, T* ext) functions to register an already existing C++ object in v8pp.

So working test2() function would look like below. Also please note, that modified test2() function returns the result by reference.

static X& test2(v8::Isolate* isolate)
{
	// create a wrapped C++ object
	v8::Local<v8::Object> x = v8pp::class_<X>::create_object(isolate, true);
	// return a reference to the object
	return v8pp::from_v8<X>(isolate, x);

	// another option: to create an object and import it to v8pp
	/*
	X* x = new X(true);
	v8pp::class_<X>::import_external(isolate, x);
	return *x;
	*/
}

@jcmonnin
Copy link
Contributor Author

Hi Pavel ,

Thanks a lot for the detailed explanation. It's very clear now what is going on.

In the real code, functions with similar structure than test2() are used in many places in the C++ code (outside of any JavaScript), so I can't modify them. I have to write helper functions for them like:

v8::Handle<v8::Object> test3(v8::Isolate* isolate)
{
    return v8pp::class_<X>::import_external(isolate, new X(test2()));
}

It is clear that when a C++ function returns an object by value, it has to be copied or moved into a heap allocated object, which the helper function does. The objects returned by the C++ functions are cheap to copy or move (they should be if they are returned by value).

In our code there are a lot of function returning objects by value. Writing all these helper functions is going to be tedious.

When binding a function that returns an object by value, I can't see another sensible behaviour for the binding. Given it's a temporary, it's certainly hasn't been wrapped in JavaScript yet. So the only option I see is to make a heap allocated copy that is owned by v8.

I have the impression that v8pp could be modified to have this behaviour as default, rather than returning undefined. Would you accept a patch providing this functionality? (I'm not sure I'm going to succeed, my template metaprogramming experience is rather limited).

@pmed
Copy link
Owner

pmed commented Jan 13, 2017

I think it's possible to add something similar to Call Policies from Boost.Python to setup how to a C++ function result would be converted. Indeed, this approach will require more of template magic, and yes, I will accept this patch.

If you wrap simple C++ structures with no member function bound, you can use specializations of v8pp::convert template for such structures: https://github.com/pmed/v8pp/blob/master/docs/convert.md#user-defined-types (there is also another example in #31)

@jcmonnin jcmonnin changed the title Question: returning object in static function Question: function that returns object by value Jan 13, 2017
@tim-janik
Copy link
Contributor

I've also been running into functions that return temporaries and came to the same conclusion, they should be heap-copied and wrapped. So I gave it a shot at patching v8pp for this in a local branch:
https://github.com/tim-janik/v8pp/tree/auto-wrap-temporaries

The actual changes here look surprisingly simple:
tim-janik@660a04a

@pmed Is it really that simple, of did I miss something in this implementation?

@pmed
Copy link
Owner

pmed commented Feb 24, 2017

@tim-janik, yes this implementation is possible, but I prefer to have an explicit way to specify the returned object should be copied. And it seems this implementation don't work when a C++ class has deleted copy constructor.

@tim-janik
Copy link
Contributor

Thanks for the response @pmed. That leaves me with two questions:

  1. What solution do you have in mind for types that with deleted copy ctor?
  2. If this is not what v8pp is going to provide, is there a way I could maintain this functionality out of v8pp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants