src: allow references to be copyable in APIs

allow non-copyable callbacks be finalizer parameters Fixes: #301 PR-URL: #915 Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs · Mar 10, 2021 · 74ab50c · 74ab50c
1 parent 929709d
commit 74ab50c
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 8 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -1345,7 +1345,8 @@ inline bool Object::InstanceOf(const Function& constructor) const {
 template <typename Finalizer, typename T>
 inline void Object::AddFinalizer(Finalizer finalizeCallback, T* data) {
   details::FinalizeData<T, Finalizer>* finalizeData =
-    new details::FinalizeData<T, Finalizer>({ finalizeCallback, nullptr });
+      new details::FinalizeData<T, Finalizer>(
+          {std::move(finalizeCallback), nullptr});
   napi_status status =
       details::AttachData(_env,
                           *this,
@@ -1363,7 +1364,8 @@ inline void Object::AddFinalizer(Finalizer finalizeCallback,
                                  T* data,
                                  Hint* finalizeHint) {
   details::FinalizeData<T, Finalizer, Hint>* finalizeData =
-    new details::FinalizeData<T, Finalizer, Hint>({ finalizeCallback, finalizeHint });
+      new details::FinalizeData<T, Finalizer, Hint>(
+          {std::move(finalizeCallback), finalizeHint});
   napi_status status =
       details::AttachData(_env,
                           *this,
@@ -1395,7 +1397,8 @@ inline External<T> External<T>::New(napi_env env,
                                     Finalizer finalizeCallback) {
   napi_value value;
   details::FinalizeData<T, Finalizer>* finalizeData =
-    new details::FinalizeData<T, Finalizer>({ finalizeCallback, nullptr });
+      new details::FinalizeData<T, Finalizer>(
+          {std::move(finalizeCallback), nullptr});
   napi_status status = napi_create_external(
     env,
     data,
@@ -1417,7 +1420,8 @@ inline External<T> External<T>::New(napi_env env,
                                     Hint* finalizeHint) {
   napi_value value;
   details::FinalizeData<T, Finalizer, Hint>* finalizeData =
-    new details::FinalizeData<T, Finalizer, Hint>({ finalizeCallback, finalizeHint });
+      new details::FinalizeData<T, Finalizer, Hint>(
+          {std::move(finalizeCallback), finalizeHint});
   napi_status status = napi_create_external(
     env,
     data,
@@ -1509,7 +1513,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
                                     Finalizer finalizeCallback) {
   napi_value value;
   details::FinalizeData<void, Finalizer>* finalizeData =
-    new details::FinalizeData<void, Finalizer>({ finalizeCallback, nullptr });
+      new details::FinalizeData<void, Finalizer>(
+          {std::move(finalizeCallback), nullptr});
   napi_status status = napi_create_external_arraybuffer(
     env,
     externalData,
@@ -1533,7 +1538,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
                                     Hint* finalizeHint) {
   napi_value value;
   details::FinalizeData<void, Finalizer, Hint>* finalizeData =
-    new details::FinalizeData<void, Finalizer, Hint>({ finalizeCallback, finalizeHint });
+      new details::FinalizeData<void, Finalizer, Hint>(
+          {std::move(finalizeCallback), finalizeHint});
   napi_status status = napi_create_external_arraybuffer(
     env,
     externalData,
@@ -2153,7 +2159,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
                                 Finalizer finalizeCallback) {
   napi_value value;
   details::FinalizeData<T, Finalizer>* finalizeData =
-    new details::FinalizeData<T, Finalizer>({ finalizeCallback, nullptr });
+      new details::FinalizeData<T, Finalizer>(
+          {std::move(finalizeCallback), nullptr});
   napi_status status = napi_create_external_buffer(
     env,
     length * sizeof (T),
@@ -2177,7 +2184,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
                                 Hint* finalizeHint) {
   napi_value value;
   details::FinalizeData<T, Finalizer, Hint>* finalizeData =
-    new details::FinalizeData<T, Finalizer, Hint>({ finalizeCallback, finalizeHint });
+      new details::FinalizeData<T, Finalizer, Hint>(
+          {std::move(finalizeCallback), finalizeHint});
   napi_status status = napi_create_external_buffer(
     env,
     length * sizeof (T),

diff --git a/test/binding.cc b/test/binding.cc
@@ -34,6 +34,7 @@ Object InitError(Env env);
 Object InitExternal(Env env);
 Object InitFunction(Env env);
 Object InitHandleScope(Env env);
+Object InitMovableCallbacks(Env env);
 Object InitMemoryManagement(Env env);
 Object InitName(Env env);
 Object InitObject(Env env);
@@ -101,6 +102,7 @@ Object Init(Env env, Object exports) {
   exports.Set("function", InitFunction(env));
   exports.Set("name", InitName(env));
   exports.Set("handlescope", InitHandleScope(env));
+  exports.Set("movable_callbacks", InitMovableCallbacks(env));
   exports.Set("memory_management", InitMemoryManagement(env));
   exports.Set("object", InitObject(env));
 #ifndef NODE_ADDON_API_DISABLE_DEPRECATED

diff --git a/test/binding.gyp b/test/binding.gyp
@@ -25,6 +25,7 @@
         'external.cc',
         'function.cc',
         'handlescope.cc',
+        'movable_callbacks.cc',
         'memory_management.cc',
         'name.cc',
         'object/delete_property.cc',

diff --git a/test/movable_callbacks.cc b/test/movable_callbacks.cc
@@ -0,0 +1,23 @@
+#include "napi.h"
+
+using namespace Napi;
+
+Value createExternal(const CallbackInfo& info) {
+  FunctionReference ref = Reference<Function>::New(info[0].As<Function>(), 1);
+  auto ret = External<char>::New(
+      info.Env(),
+      nullptr,
+      [ref = std::move(ref)](Napi::Env /*env*/, char* /*data*/) {
+        ref.Call({});
+      });
+
+  return ret;
+}
+
+Object InitMovableCallbacks(Env env) {
+  Object exports = Object::New(env);
+
+  exports["createExternal"] = Function::New(env, createExternal);
+
+  return exports;
+}
diff --git a/test/movable_callbacks.js b/test/movable_callbacks.js
@@ -0,0 +1,27 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const common = require('./common');
+const testUtil = require('./testUtil');
+
+Promise.all([
+  test(require(`./build/${buildType}/binding.node`).movable_callbacks),
+  test(require(`./build/${buildType}/binding_noexcept.node`).movable_callbacks),
+]).catch(e => {
+  console.error(e);
+  process.exitCode = 1;
+});
+
+async function test(binding) {
+  await testUtil.runGCTests([
+    'External',
+    () => {
+      const fn = common.mustCall(() => {
+        // noop
+      }, 1);
+      const external = binding.createExternal(fn);
+    },
+    () => {
+      // noop, wait for gc
+    }
+  ]);
+}