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

Fix misleading documentation concerning finalize callbacks #384

Closed
wants to merge 1 commit into from

Conversation

computerquip
Copy link

Documentation fixes as discussed in #383

destroyed. It must implement `operator()`, accept a `void*` (which is the
`externalData` pointer), and return `void`.
destroyed. It must implement `operator()` with parameters `(Napi::Env, void*)`,
and return `void`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we missed a couple of parameters I think the signature is

static void FinalizeCallback(napi_env env, void* data, void* hint);

@computerquip any chance you can update to include the last parameter as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson:
From N-API napi_finalizer function pointer type always has 3 parameter, but in node-addon-api it's different.

  • for ArrayBuffer::New(..., finalizeCallback, finalizeHint) we have Finalizer (Env, void* data, Hint* hint), like napi_finalizer
  • for ArrayBuffer::New(..., finalizeCallback) we have Finalizer (Env, void* data)
template <typename T, typename Finalizer, typename Hint = void>
struct FinalizeData {
  static inline
  void Wrapper(napi_env env, void* data, void* finalizeHint) {
    FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
    finalizeData->callback(Env(env), static_cast<T*>(data));
    delete finalizeData;
  }

  static inline
  void WrapperWithHint(napi_env env, void* data, void* finalizeHint) {
    FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
    finalizeData->callback(Env(env), static_cast<T*>(data), finalizeData->hint);
    delete finalizeData;
  }

  Finalizer callback;
  Hint* hint;
};

DaAitch added a commit to DaAitch/node-addon-api that referenced this pull request Dec 26, 2018
- External::New
- ArrayBuffer::New
- Buffer::New

Ref: nodejs#384
Fixes: nodejs#383
@computerquip
Copy link
Author

computerquip commented Dec 29, 2018

Closing since this appears to be adopted in another pull request: #414

mhdawson pushed a commit that referenced this pull request Jan 2, 2019
- External::New
- ArrayBuffer::New
- Buffer::New

PR-URL: #414
Fixes: #383
Refs: #384
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
- External::New
- ArrayBuffer::New
- Buffer::New

PR-URL: nodejs/node-addon-api#414
Fixes: nodejs/node-addon-api#383
Refs: nodejs/node-addon-api#384
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
- External::New
- ArrayBuffer::New
- Buffer::New

PR-URL: nodejs/node-addon-api#414
Fixes: nodejs/node-addon-api#383
Refs: nodejs/node-addon-api#384
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
- External::New
- ArrayBuffer::New
- Buffer::New

PR-URL: nodejs/node-addon-api#414
Fixes: nodejs/node-addon-api#383
Refs: nodejs/node-addon-api#384
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
- External::New
- ArrayBuffer::New
- Buffer::New

PR-URL: nodejs/node-addon-api#414
Fixes: nodejs/node-addon-api#383
Refs: nodejs/node-addon-api#384
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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

Successfully merging this pull request may close these issues.

3 participants