search

pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
890 stars 120 forks source link

Calling functions taking ref or raw ptr when using `shared_ptr_traits` #79

Open jcmonnin opened 6 years ago

jcmonnin commented 6 years ago

I bind my C++ types with shared_ptr_traits when there is shared ownership in between C++ and JavaScript or if I need to pass ownership from JavaScript to C++. This works well, but I have to apply some workarounds. When having such a type bound with shared_ptr, it's quite common to have to call API's that expect an object reference or a raw pointer. This currently doesn't work with v8pp.

Here is a short summary of the main issue: When a class is bound to JavaScript with shared_ptr

v8pp::class_<Test, v8pp::shared_ptr_traits> testClass(isolate);

we cannot call a function taking a reference:

void myFunction(Test const& test);

More complete example:

class Test : std::enable_shared_from_this<Test>
{
public:
    Test(int i) : i_(i) {}

    int i() const { return i_; }
    void setI(int i) { i_ = i; }

private:
    int i_;
};

std::shared_ptr<Test const> printTestSPtr(std::shared_ptr<Test const> test)
{
    if (test) {
        std::cout << test->i() << std::endl;
    } else {
        std::cout << "nullptr" << std::endl;
    }
    return test;
}

const Test* printTestRawPtr(const Test* test)
{
    if (test) {
        std::cout << test->i() << std::endl;
    } else {
        std::cout << "nullptr" << std::endl;
    }
    return test;
}

Test printTestRef(Test const& test)
{
    std::cout << test.i() << std::endl;
    return test;
}

template<typename Traits>
void bindTest(v8::Isolate* isolate)
{
    v8pp::class_<Test, Traits> testClass(isolate);
    testClass
        .template ctor<int>()
        .set("i", &Test::i)
        .set("setI", &Test::setI);

    v8pp::module myLib(isolate);
    myLib
        .set("Test", testClass)
        .set("printTestSPtr", printTestSPtr)
        .set("printTestRawPtr", printTestRawPtr)
        .set("printTestRef", printTestRef);

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

Using v8pp::shared_ptr_traits

When initialising the context with bindTest<v8pp::shared_ptr_traits>(isolate);, I get following results (note: the situation with the return values is there for completeness, but not the main issue):

Calling function taking shared_ptr

let x = new myLib.Test(123);
myLib.printTestSPtr(x).i()

gives the expected result (first line is std::cout, 2nd line is what the script expression returns)

123
123

Calling function taking raw pointer

let x = new myLib.Test(123);
myLib.printTestRawPtr(x).i()

When calling function taking raw pointer, I get a C++ exception. Ideally, this would work. Admittedly, the return value is a bit more complicated. I guess it would be ok if it throws a C++ exception when doing the return value (although theoretically it could get a shared_prt instance if the class derives from std::enable_shared_from_this).

Exception: Error: v8pp::class_<Test, v8pp::shared_ptr_traits> is already registered in isolate 0x107000000 before of v8pp::class_<Test, v8pp::raw_ptr_traits>

Calling function taking reference

let x = new myLib.Test(123);
myLib.printTestRef(x).i()

When calling function taking a reference, I get a C++ exception. This is a quite common case which should work. The return value situation is #39.

Exception: Error: v8pp::class_<Test, v8pp::shared_ptr_traits> is already registered in isolate 0x10200be00 before of v8pp::class_<Test, v8pp::raw_ptr_traits>

Using v8pp::raw_ptr_traits

As a comparison, here is what I get when initialising the context withbindTest<v8pp::raw_ptr_traits>(isolate);:

Calling function taking shared_ptr

let x = new myLib.Test(123);
myLib.printTestSPtr(x).i()

This gives a C++ exception. Generally, this call makes no sense. Strictly speaking this would be possible if the class derives from std::enable_shared_from_this, however this use case is probably not relevant in practice.

Exception: Error: v8pp::class_<Test, v8pp::raw_ptr_traits> is already registered in isolate 0x104803800 before of v8pp::class_<Test, v8pp::shared_ptr_traits>

Calling function taking raw pointer

let x = new myLib.Test(123);
myLib.printTestRawPtr(x).i()

gives the expected result

123
123

Calling function taking reference

let x = new myLib.Test(123);
myLib.printTestRef(x).i()

Calling a reference is possible. Return value issue is #39. It looks like earlier versions of v8pp were returning undefined whereas now it throws a C++ exception.

123
Exception: Error: failed to wrap C++ object

Current workaround

To solve the main issue of calling a function taking a reference or pointer as argument, I have a very ugly workaround. When I comment out the traits check in classes::find, then it seems to do what I want. The problem is that this relies on undefined behaviour (I don't think that it's safe to convert an instance to a different type with static_cast). So I would be keen to have a more permanent solution that this:

    template<typename Traits>
    static object_registry<Traits>& find(v8::Isolate* isolate, type_info const& type)
    {
        classes* info = instance(operation::get, isolate);
        type_info const& traits = type_id<Traits>();
        if (info)
        {
            auto it = info->find(type);
            if (it != info->classes_.end())
            {
//                if ((*it)->traits != traits)
//                {
//                    throw std::runtime_error((*it)->class_name()
//                        + " is already registered in isolate "
//                        + pointer_str(isolate) + " before of "
//                        + class_info(type, traits).class_name());
//                }
                return *static_cast<object_registry<Traits>*>(it->get());
            }
        }
        //assert(false && "class not registered");
        throw std::runtime_error(class_info(type, traits).class_name()
            + " is not registered in isolate " + pointer_str(isolate));
    }
pmed commented 6 years ago

Hi Jean-Claude,

Thank you for the detailed issue description.

References to wrapped class as function arguments should be converted with convert_ref in ptrtraits. There is a case in the [`v8pp::class` test](https://github.com/pmed/v8pp/blob/master/test/test_class.cpp#L49).

The cause of issue could be due to const qualifiers for the argument references. I will add your code into the test case in order to check it.

This checking of traits types in find() function was added to prevent implicit conversions between different pointer types. But if such conversions seems to be handy, we could try to enable them. Maybe by adding helper functions into the traits classes for raw_ptr_traits to shared_ptr_traits and vice versa, similar to convert_ref templates.

Best regards, Pavel

jcmonnin commented 6 years ago

Hi Pavel,

Thanks for the reply. You are right, calling a function taking a reference actually does work. I was taking wrong conclusions for some reason. I clearly tried to add two many things in my test case.

The problem with the call to a function/method taking a raw pointer remains.

Given my initial post is too big and contains wrong things, I propose to close it and I will reopen another one with a more concise example about it. Is that ok or do you want to continue on this issue number?

Thanks, Jean-Claude

pmed commented 6 years ago

Hi Jean-Claude,

I think it’s ok to continue in this issue, thank you.

чт, 19 июля 2018 г. в 12:04, Jean-Claude Monnin notifications@github.com:

Hi Pavel,

Thanks for the reply. You are right, calling a function taking a reference actually does work. I was taking wrong conclusions for some reason. I clearly tried to add two many things in my test case.

The problem with the call to a function/method taking a raw pointer remains.

Given my initial post is too big and contains wrong things, I propose to close it and I will reopen another one with a more concise example about it. Is that ok or do you want to continue on this issue number?

Thanks, Jean-Claude

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/pmed/v8pp/issues/79#issuecomment-406224748, or mute the thread https://github.com/notifications/unsubscribe-auth/ABEgUrvI45-9cMaEFbiP5ZDCQGDTOWtCks5uIFmwgaJpZM4VO1PH .

-- Sincerely, Pavel

jcmonnin commented 5 years ago

Hi Pavel,

I'd like to come back to calling a function taking a raw pointer (when the class has been declared with v8pp::shared_ptr_traits). For the example above this would be a function like:

void printTestRawPtr(const Test* test)

This fails because unwrap_object calls classes::find which throws exception because of mismatching traits.

In the API I need to bind these type of functions are pretty common and I need to bind my classes to JavaScript with v8pp::shared_ptr_traits because there is no other clean way to express the ownership if the instances.

As said in the initial comment, commenting out some lines in classes::find make the test case go through, but the implementation is not clean (I think it would be undefined behaviour to cast the instance to a different type than what it has been instantiated):

    template<typename Traits>
    static object_registry<Traits>& find(v8::Isolate* isolate, type_info const& type)
    {
        classes* info = instance(operation::get, isolate);
        type_info const& traits = type_id<Traits>();
        if (info)
        {
            auto it = info->find(type);
            if (it != info->classes_.end())
            {
//                if ((*it)->traits != traits)
//                {
//                    throw std::runtime_error((*it)->class_name()
//                        + " is already registered in isolate "
//                        + pointer_str(isolate) + " before of "
//                        + class_info(type, traits).class_name());
//                }
                return *static_cast<object_registry<Traits>*>(it->get());
            }
        }
        //assert(false && "class not registered");
        throw std::runtime_error(class_info(type, traits).class_name()
            + " is not registered in isolate " + pointer_str(isolate));
    }

If you see a strait-forward way to fix it, please let me know.

Otherwise maybe the suggestions below are helpful (hopefully it's not too much off topic);

In the old days, I was using luabind to bind C++ classes lua code (it was pre C+11, so this was with boost::shared_ptr). It was handling the case above. Nowadays sol2 is the reference for binding lua code and it has very flexible support for smart pointers. It might be worthwhile to have a look there:

http://luabind.sourceforge.net/docs.html#class_smart_pointers http://sol2.readthedocs.io/en/latest/api/unique_usertype_traits.html

When a smart pointer is used, the smart pointer is not replacement of the raw pointer as in v8pp, but an additional concept (holder_type/actual_type). The traits are defined independently of the wrapper class. This allows more flexibility. It's not necessary to make the choice to either use shared_ptr or raw pointers everywhere. It's possible to mix things more freely (and also supports other smart pointer types than std::shared_ptr):

http://sol2.readthedocs.io/en/latest/tutorial/all-the-things.html#c-classes-from-c

Possibly this doesn't really translate to v8pp/JavaScript, but maybe you find some of the ideas useful.