Merge lp:~marcustomlinson/v8-cpp/test-gc into lp:v8-cpp

[Marcus Tomlinson]

Added support for shared_ptr and unique_ptr

[Alexandre Abreu (abreu-alexandre)]

small comment inline

[Marcus Tomlinson (marcustomlinson)]

Replied to inline comments.

[Alexandre Abreu (abreu-alexandre)]

replied

[Marcus Tomlinson (marcustomlinson)]

> replied

Ok I see what you're saying. I'll try specifically set the internal field counts to 2 and 3 for non-shared_ptr and shared_ptr objects respectively.

[Marcus Tomlinson (marcustomlinson)]

Hmmm, so I've untangled my thoughts and realised again why it has to remain implemented this way.

The internal field count is set when creating the class template. This template is what we use to mould instances of the class as they are created. We only manage exported class types, even in the case of smart pointers, we are actually instantiating the underlying class type template and appending the extra smart pointer reference as the 3rd internal field.

Seeing that we need our class templates to support both normal instances as well as smart pointer instances, we have to make that 3rd internal field available in all of our templates as either variant may be constructed from them.

Hope I'm making sense?

[Alexandre Abreu (abreu-alexandre)]

> Hmmm, so I've untangled my thoughts and realised again why it has to remain
> implemented this way.
>
> The internal field count is set when creating the class template. This
> template is what we use to mould instances of the class as they are created.
> We only manage exported class types, even in the case of smart pointers, we
> are actually instantiating the underlying class type template and appending
> the extra smart pointer reference as the 3rd internal field.
>
> Seeing that we need our class templates to support both normal instances as
> well as smart pointer instances, we have to make that 3rd internal field
> available in all of our templates as either variant may be constructed from
> them.
>
> Hope I'm making sense?

totally,

mmh changing that would require quite a bit of "redoing", we might as well leave this as is, and update it if necessary,

[Alexandre Abreu (abreu-alexandre)]

review: Approve

=== modified file 'src/internal/class.h'
--- src/internal/class.h	2015-08-05 18:57:44 +0000
+++ src/internal/class.h	2015-09-01 09:57:53 +0000
@@ -22,6 +22,12 @@
 #include <internal/function.h>
 #include <internal/utility/object_factory.h>
 
+#include <memory>
+
+template <typename T> struct IsSharedPointer : std::false_type {};
+template <typename T> struct IsSharedPointer<std::shared_ptr<T>> : std::true_type { using type = T; };
+template <typename T> struct IsSharedPointer<std::shared_ptr<T const>> : std::true_type { using type = T; };
+
 namespace v8cpp
 {
 namespace internal
@@ -44,7 +50,7 @@
         }
 
         // Find Class instance by class type
-        size_t my_type = class_type_();
+        size_t my_type = class_type();
         Class* result;
         if (my_type < instances->size())
         {
@@ -96,50 +102,111 @@
         proto_template()->Inherit(base_class->class_template());
     }
 
-    v8::Local<v8::Object> export_object(T* object)
+    template <typename O>
+    typename std::enable_if<IsSharedPointer<O>::value, v8::Local<v8::Object>>::type export_object(O* object, bool)
     {
         v8::EscapableHandleScope scope(isolate_);
 
-        v8::Local<v8::Object> v8_object = class_template()->GetFunction()->NewInstance();
-        v8_object->SetAlignedPointerInInternalField(0, object);
-        v8_object->SetAlignedPointerInInternalField(1, this);
+        using raw_ptr_class = Class<typename IsSharedPointer<T>::type>;
+        v8::Local<v8::Object> v8_object = raw_ptr_class::instance(isolate_).class_template()->GetFunction()->NewInstance();
+        v8_object->SetAlignedPointerInInternalField(0, object->get());
+        v8_object->SetAlignedPointerInInternalField(1, &raw_ptr_class::instance(isolate_));
+        v8_object->SetAlignedPointerInInternalField(2, object);
 
         MoveablePersistent<v8::Object> v8_object_p(isolate_, v8_object);
         v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
                             {
                                 v8::Isolate* isolate = data.GetIsolate();
                                 T* object = data.GetParameter();
-                                instance(isolate).destroy_object(object);
+                                raw_ptr_class::instance(isolate).remove_object<T>(isolate, object, &ObjectFactory<T>::delete_object);
                             });
 
-        ClassInfo::add_object(object, std::move(v8_object_p));
-
-        return scope.Escape(v8_object);
-    }
-
-    v8::Local<v8::Object> export_object(T const* object)
-    {
-        return export_object(const_cast<T*>(object));
+        raw_ptr_class::instance(isolate_).add_object(object, std::move(v8_object_p));
+
+        return scope.Escape(v8_object);
+    }
+
+    template <typename O>
+    typename std::enable_if<!IsSharedPointer<O>::value, v8::Local<v8::Object>>::type export_object(O* object, bool gc)
+    {
+        v8::EscapableHandleScope scope(isolate_);
+
+        v8::Local<v8::Object> v8_object = class_template()->GetFunction()->NewInstance();
+        v8_object->SetAlignedPointerInInternalField(0, object);
+        v8_object->SetAlignedPointerInInternalField(1, this);
+        v8_object->SetAlignedPointerInInternalField(2, nullptr);
+
+        MoveablePersistent<v8::Object> v8_object_p(isolate_, v8_object);
+        if (gc)
+        {
+            v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
+                                {
+                                    v8::Isolate* isolate = data.GetIsolate();
+                                    T* object = data.GetParameter();
+                                    instance(isolate).remove_object<T>(isolate, object, &ObjectFactory<T>::delete_object);
+                                });
+        }
+        else
+        {
+            v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
+                                {
+                                    v8::Isolate* isolate = data.GetIsolate();
+                                    T* object = data.GetParameter();
+                                    instance(isolate).remove_object<T>(isolate, object, nullptr);
+                                });
+        }
+
+        add_object(object, std::move(v8_object_p));
+
+        return scope.Escape(v8_object);
+    }
+
+    v8::Local<v8::Object> export_object(T const* object, bool gc)
+    {
+        return export_object(const_cast<T*>(object), gc);
     }
 
     v8::Local<v8::Object> export_object(v8::FunctionCallbackInfo<v8::Value> const& args)
     {
-        return constructor_ ? export_object(constructor_(args)) :
+        return constructor_ ? export_object(constructor_(args), true) :
                               throw std::runtime_error("exported class does not have a constructor specified");
     }
 
-    T* import_object(v8::Local<v8::Value> value)
-    {
-        v8::HandleScope scope(isolate_);
-
-        while (value->IsObject())
-        {
-            v8::Local<v8::Object> object = value->ToObject();
-            if (object->InternalFieldCount() == 2)
-            {
-                void* ptr = object->GetAlignedPointerFromInternalField(0);
-                ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
-                if (info && info->cast(ptr, class_type_()))
+    template <typename O>
+    typename std::enable_if<IsSharedPointer<O>::value, O*>::type import_object(v8::Local<v8::Value> value)
+    {
+        v8::HandleScope scope(isolate_);
+
+        while (value->IsObject())
+        {
+            v8::Local<v8::Object> object = value->ToObject();
+            if (object->InternalFieldCount() == 3)
+            {
+                void* ptr = object->GetAlignedPointerFromInternalField(0);
+                ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
+                if (info && info->cast(ptr, Class<typename IsSharedPointer<T>::type>::instance(isolate_).class_type()))
+                {
+                    return static_cast<T*>(object->GetAlignedPointerFromInternalField(2));
+                }
+            }
+            value = object->GetPrototype();
+        }
+        return nullptr;
+    }
+
+    template <typename O>
+    typename std::enable_if<!IsSharedPointer<O>::value, O*>::type import_object(v8::Local<v8::Value> value)
+    {
+        v8::HandleScope scope(isolate_);
+
+        while (value->IsObject())
+        {
+            v8::Local<v8::Object> object = value->ToObject();
+            if (object->InternalFieldCount() == 3)
+            {
+                void* ptr = object->GetAlignedPointerFromInternalField(0);
+                ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
+                if (info && info->cast(ptr, class_type()))
                 {
                     return static_cast<T*>(ptr);
                 }
@@ -149,11 +216,6 @@
         return nullptr;
     }
 
-    void destroy_object(T* object)
-    {
-        ClassInfo::remove_object(isolate_, object, &ObjectFactory<T>::delete_object);
-    }
-
     template <typename P>
     static void get_member(v8::Local<v8::String>, v8::PropertyCallbackInfo<v8::Value> const& info)
     {
@@ -177,6 +239,12 @@
         self.*property_ptr = from_v8<PropType>(isolate, value);
     }
 
+    static size_t class_type()
+    {
+        static size_t my_type = ClassInfo::register_class();
+        return my_type;
+    }
+
 private:
     explicit Class(v8::Isolate* isolate, size_t type)
         : ClassInfo(type)
@@ -205,13 +273,8 @@
 
         // Each JS instance has a built-in field holding a pointer to the C++ object
         // We add a second field to hold a pointer to this Class instance
-        class_template->InstanceTemplate()->SetInternalFieldCount(2);
-    }
-
-    static size_t class_type_()
-    {
-        static size_t my_type = ClassInfo::register_class();
-        return my_type;
+        // And an optional third field to hold a smart pointer
+        class_template->InstanceTemplate()->SetInternalFieldCount(3);
     }
 
     v8::Isolate* isolate_;

=== modified file 'src/internal/convert.h'
--- src/internal/convert.h	2015-08-05 18:57:44 +0000
+++ src/internal/convert.h	2015-09-01 09:57:53 +0000
@@ -21,6 +21,7 @@
 #include <climits>
 #include <vector>
 #include <map>
+#include <memory>
 
 #include <v8.h>
 
@@ -369,40 +370,38 @@
 
 // Exported class converters
 template <typename T>
-struct IsExportedClass : std::is_class<T>
-{
-};
-
-template <typename T>
-struct IsExportedClass<v8::Local<T>> : std::false_type
-{
-};
-
-template <typename T>
-struct IsExportedClass<v8::Persistent<T>> : std::false_type
-{
-};
+struct IsExportedClass : std::is_class<T> {};
+
+template <typename T>
+struct IsExportedClass<v8::Local<T>> : std::false_type {};
+
+template <typename T>
+struct IsExportedClass<v8::Persistent<T>> : std::false_type {};
 
 template <typename Char, typename Traits, typename Alloc>
-struct IsExportedClass<std::basic_string<Char, Traits, Alloc>> : std::false_type
-{
-};
+struct IsExportedClass<std::basic_string<Char, Traits, Alloc>> : std::false_type {};
 
 template <typename T, typename Alloc>
-struct IsExportedClass<std::vector<T, Alloc>> : std::false_type
-{
-};
+struct IsExportedClass<std::vector<T, Alloc>> : std::false_type {};
 
 template <typename Key, typename Value, typename Less, typename Alloc>
-struct IsExportedClass<std::map<Key, Value, Less, Alloc>> : std::false_type
-{
-};
+struct IsExportedClass<std::map<Key, Value, Less, Alloc>> : std::false_type {};
+
+template <typename T>
+struct IsUniquePointer : std::false_type {};
+
+template <typename T>
+struct IsUniquePointer<std::unique_ptr<T>> : std::true_type { using type = T; };
+
+template <typename T>
+struct IsUniquePointer<std::unique_ptr<T const>> : std::true_type { using type = T; };
 
 template <typename T>
 struct Convert<T, typename std::enable_if<IsExportedClass<T>::value>::type>
 {
     using FromType = T&;
     using ToType = v8::Local<v8::Object>;
+    using ClassType = typename std::remove_cv<T>::type;
 
     static bool is_valid(v8::Isolate*, v8::Local<v8::Value> value)
     {
@@ -422,9 +421,18 @@
         throw std::runtime_error("expected an exported object");
     }
 
-    static ToType to_v8(v8::Isolate* isolate, T const& value)
-    {
-        return Convert<T*>::to_v8(isolate, new T(value));
+    template <typename O>
+    static typename std::enable_if<IsUniquePointer<O>::value, ToType>::type to_v8(v8::Isolate* isolate, O const& value)
+    {
+        // Convert unique pointers to shared pointers before exposing them to v8
+        std::shared_ptr<typename IsUniquePointer<T>::type> s_ptr{std::move(const_cast<T&>(value))};
+        return Convert<std::shared_ptr<typename IsUniquePointer<T>::type>>::to_v8(isolate, s_ptr);
+    }
+
+    template <typename O>
+    static typename std::enable_if<!IsUniquePointer<O>::value, ToType>::type to_v8(v8::Isolate* isolate, O const& value)
+    {
+        return Class<ClassType>::instance(isolate).export_object(new T(value), true);
     }
 };
 
@@ -446,12 +454,12 @@
         {
             throw std::invalid_argument("expected an object");
         }
-        return Class<ClassType>::instance(isolate).import_object(value);
+        return Class<ClassType>::instance(isolate).template import_object<T>(value);
     }
 
     static ToType to_v8(v8::Isolate* isolate, T const* value)
     {
-        return Class<ClassType>::instance(isolate).export_object(value);
+        return Class<ClassType>::instance(isolate).export_object(value, false);
     }
 };
 

=== modified file 'src/run.h'
--- src/run.h	2015-08-05 18:57:44 +0000
+++ src/run.h	2015-09-01 09:57:53 +0000
@@ -32,6 +32,11 @@
     // Create an isolate
     std::shared_ptr<v8::Isolate> isolate(v8::Isolate::New(), [](v8::Isolate* isolate)
     {
+        // Force garbage collection before returning
+        std::string const v8_flags = "--expose_gc";
+        v8::V8::SetFlagsFromString(v8_flags.data(), (int)v8_flags.length());
+        isolate->RequestGarbageCollectionForTesting(v8::Isolate::GarbageCollectionType::kFullGarbageCollection);
+
         // Clean up
         using ClassInstances = std::vector<v8cpp::internal::Class<void>*>;
         ClassInstances* instances = static_cast<ClassInstances*>(isolate->GetData(0));

=== modified file 'tests/objects/module.cpp'
--- tests/objects/module.cpp	2015-08-05 18:57:44 +0000
+++ tests/objects/module.cpp	2015-09-01 09:57:53 +0000
@@ -32,6 +32,8 @@
     testclass
             .set_constructor<int, int>()
             .add_method("i", &TestClass::i)
+            .add_method("embedded_class_sptr", &TestClass::embedded_class_sptr)
+            .add_method("embedded_class_uptr", &TestClass::embedded_class_uptr)
             .add_method("embedded_class_ptr", &TestClass::embedded_class_ptr)
             .add_method("embedded_class_ref", &TestClass::embedded_class_ref)
             .add_method("embedded_class_copy", &TestClass::embedded_class_copy)

=== modified file 'tests/objects/test.cpp'
--- tests/objects/test.cpp	2015-07-08 09:29:21 +0000
+++ tests/objects/test.cpp	2015-09-01 09:57:53 +0000
@@ -54,32 +54,41 @@
 
 TEST(Test, object_to_js)
 {
-    auto test_object = v8cpp::run_script<EmbeddedTestClass*>(
+    auto test_object = v8cpp::run_script<std::shared_ptr<EmbeddedTestClass>>(
+    R"(
+        var module = require("./test-objects-module");
+        var test_object = module.new_TestClass(1, 2);
+        test_object.embedded_class_uptr();
+    )");
+
+    EXPECT_EQ(test_object->i(), -1);
+
+    auto test_object2 = v8cpp::run_script<std::shared_ptr<EmbeddedTestClass>>(
+    R"(
+        var module = require("./test-objects-module");
+        var test_object = module.new_TestClass(1, 2);
+        test_object.embedded_class_sptr();
+    )");
+
+    EXPECT_EQ(test_object2->i(), -1);
+
+    auto test_object3 = v8cpp::run_script<EmbeddedTestClass*>(
     R"(
         var module = require("./test-objects-module");
         var test_object = module.new_TestClass(1, 2);
         test_object.embedded_class_ptr();
     )");
 
-    EXPECT_EQ(test_object->i(), -1);
-
-    auto test_object2 = v8cpp::run_script<EmbeddedTestClass&>(
-    R"(
-        var module = require("./test-objects-module");
-        var test_object = module.new_TestClass(1, 2);
-        test_object.embedded_class_ref();
-    )");
-
-    EXPECT_EQ(test_object2.i(), -1);
-
-    auto test_object3 = v8cpp::run_script<EmbeddedTestClass>(
+    EXPECT_EQ(test_object3->i(), -1);
+
+    auto test_object4 = v8cpp::run_script<EmbeddedTestClass>(
     R"(
         var module = require("./test-objects-module");
         var test_object = module.new_TestClass(1, 2);
         test_object.embedded_class_copy();
     )");
 
-    EXPECT_EQ(test_object3.i(), -1);
+    EXPECT_EQ(test_object4.i(), -1);
 }
 
 TEST(Test, object_from_js)
@@ -94,9 +103,31 @@
         test_object;
     )");
 
+    auto result = v8cpp::run_script<int>(
+    R"(
+        var module = require("./test-objects-module");
+        var test_object = module.new_TestClass(1, 2);
+
+        var embedded_object = test_object.embedded_class_uptr();
+        embedded_object.i();
+    )");
+
+    EXPECT_EQ(result, -1);
+
+    auto result2 = v8cpp::run_script<int>(
+    R"(
+        var module = require("./test-objects-module");
+        var test_object = module.new_TestClass(1, 2);
+
+        var embedded_object = test_object.embedded_class_sptr();
+        embedded_object.i();
+    )");
+
+    EXPECT_EQ(result2, -1);
+
     EXPECT_EQ(test_object.i(), -1);
 
-    auto result = v8cpp::run_script<int>(
+    auto result3 = v8cpp::run_script<int>(
     R"(
         var module = require("./test-objects-module");
         var test_object = module.new_TestClass(1, 2);
@@ -106,5 +137,17 @@
                           test_object2.embedded_class_ref());
     )");
 
-    EXPECT_EQ(result, -2);
+    EXPECT_EQ(result3, -2);
+
+    auto result4 = v8cpp::run_script<int>(
+    R"(
+        var module = require("./test-objects-module");
+        var test_object = module.new_TestClass(1, 2);
+        var test_object2 = module.new_TestClass(1, 2);
+
+        test_object.add_i(test_object.embedded_class_sptr(),
+                          test_object2.embedded_class_uptr());
+    )");
+
+    EXPECT_EQ(result4, -2);
 }

=== modified file 'tests/objects/test.h'
--- tests/objects/test.h	2015-07-08 06:20:26 +0000
+++ tests/objects/test.h	2015-09-01 09:57:53 +0000
@@ -20,6 +20,8 @@
 
 #include <gtest/gtest.h>
 
+#include <memory>
+
 class EmbeddedTestClass
 {
 public:
@@ -43,7 +45,7 @@
 {
 public:
     TestClass(int a, int b)
-        : embedded_class_(a, b)
+        : embedded_class_(std::make_shared<EmbeddedTestClass>(a, b))
     {
         EXPECT_EQ(a, 1);
         EXPECT_EQ(b, 2);
@@ -55,19 +57,29 @@
         return i_;
     }
 
+    std::shared_ptr<EmbeddedTestClass> embedded_class_sptr()
+    {
+        return embedded_class_;
+    }
+
+    std::unique_ptr<EmbeddedTestClass> embedded_class_uptr()
+    {
+        return std::unique_ptr<EmbeddedTestClass>(new EmbeddedTestClass(1, 2));
+    }
+
     EmbeddedTestClass* embedded_class_ptr()
     {
-        return &embedded_class_;
+        return embedded_class_.get();
     }
 
     EmbeddedTestClass& embedded_class_ref()
     {
-        return embedded_class_;
+        return *embedded_class_;
     }
 
     EmbeddedTestClass embedded_class_copy()
     {
-        return embedded_class_;
+        return *embedded_class_;
     }
 
     void replace_i(EmbeddedTestClass const& other)
@@ -81,7 +93,7 @@
     }
 
 private:
-    EmbeddedTestClass embedded_class_;
+    std::shared_ptr<EmbeddedTestClass> embedded_class_;
     int i_;
 };
 

=== modified file 'valgrind.supp'
--- valgrind.supp	2015-07-08 09:37:32 +0000
+++ valgrind.supp	2015-09-01 09:57:53 +0000
@@ -21,7 +21,8 @@
    GC not cleaning up exported objects (via to_v8)
    Memcheck:Leak
    fun:_Znwm
-   fun:_ZN5v8cpp8internal7Convert*to_v8EPN2v87IsolateERKS2_
+   fun:_ZN5v8cpp8internal7Convert*to_v8*
+   ...
    fun:_ZN5v8cpp5to_v8*
    fun:_ZN5v8cpp8internal14forward_invoke*
    fun:_ZN5v8cpp8internal16forward_function*