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

Issue 602: Add template methods to ObjectWrap #604

Closed
wants to merge 2 commits into from
Closed

Issue 602: Add template methods to ObjectWrap #604

wants to merge 2 commits into from

Conversation

dmitryash
Copy link
Contributor

@dmitryash dmitryash commented Nov 22, 2019

ObjectWrap was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap::InstanceMethod, ObjectWrap::StaticAccessor, etc.

There are several benefits:

  • no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  • a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");

Closes #602.

Use squash method.

ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Closes #602.
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Nov 22, 2019

We should consider moving the existing methods to napi-inl.deprecated.h and mark them as deprecated. Or we should use a multi-step process:

  1. Mark them as deprecated in the docs
  2. In 3.0.0, move them to napi-inl.deprecated.h
  3. In 4.0.0, remove them

@nodejs/n-api what do y'all think?

@legendecas
Copy link
Member

IMO there are no really breaking changes landed on most recent major version v2. I'd prefer the latter less aggressive pattern for the common usage on ObjectWrap: no actual breaking changes would be landed on v2.

@mhdawson
Copy link
Member

I'm good with the move to napi-inl.deprecated.h, removing completely we can discuss later.

We talked about adding a new deprecation guard so that people who already have napi_deprecated are not affected, but people can opt-in to the the higher level of deprecation.

In this case we'll use: napi_disable_deprecated_1

napi_disable_deprecated will map to napi_disabled_deprecated_0
napi_disable_deprecated_all will map to the current highest level

@gabrielschulhof
Copy link
Contributor

@mhdawson I think it would OK to land this PR as is, and then leave the move to deprecated to another PR.

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Dec 4, 2019

@gabrielschulhof
Copy link
Contributor

Here it is again:

CI against v8.x: https://ci.nodejs.org/job/node-test-node-addon-api-new/1218/

gabrielschulhof pushed a commit that referenced this pull request Dec 5, 2019
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: #602
PR-URL: #604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@gabrielschulhof
Copy link
Contributor

Landed in ce91e14.

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.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.

Avoid extra memory allocation with templates
4 participants