-
Notifications
You must be signed in to change notification settings - Fork 465
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
src: allow non-copyable callbacks be finalizer parameters #915
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
test/movable_callbacks.cc
Outdated
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails to compile for me with:
../movable_callbacks.cc:8:61: error: unused parameter ‘env’ [-Werror=unused-parameter]
info.Env(), nullptr, [ref = std::move(ref)](Napi::Env env, char* data) {
~~~~~~~~~~^~~
../movable_callbacks.cc:8:72: error: unused parameter ‘data’ [-Werror=unused-parameter]
info.Env(), nullptr, [ref = std::move(ref)](Napi::Env env, char* data) {
~~~~~~^~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixed up linter complaints as part of landing. Landed as 74ab50c |
allow non-copyable callbacks be finalizer parameters Fixes: nodejs/node-addon-api#301 PR-URL: nodejs/node-addon-api#915 Reviewed-By: Michael Dawson <midawson@redhat.com>
allow non-copyable callbacks be finalizer parameters Fixes: nodejs/node-addon-api#301 PR-URL: nodejs/node-addon-api#915 Reviewed-By: Michael Dawson <midawson@redhat.com>
allow non-copyable callbacks be finalizer parameters Fixes: nodejs/node-addon-api#301 PR-URL: nodejs/node-addon-api#915 Reviewed-By: Michael Dawson <midawson@redhat.com>
allow non-copyable callbacks be finalizer parameters Fixes: nodejs/node-addon-api#301 PR-URL: nodejs/node-addon-api#915 Reviewed-By: Michael Dawson <midawson@redhat.com>
Fixes: #301
In real-world use cases,
Napi::Reference
s are frequently been passed around by lambda captures in finalizers, callbacks. This PR aims to make those conditions easier to handle.