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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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.

@dmitryash
Issue 602: Add template methods to ObjectWrap
b88e797 
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.
@dmitryash
workaround for missing initializer warning (old gcc)
3ff5c03
gabrielschulhof
gabrielschulhof approved these changes Nov 22, 2019
View reviewed changes
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 •
edited
Loading

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 •
edited
Loading

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

@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
Copy link
Contributor

gabrielschulhof commented Dec 5, 2019 •
edited
Loading

CI:

Version Job Status
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1234/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1235/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1236/ ✓
v8.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1237/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1238/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1239/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1240/ 🛑
https://ci.nodejs.org/job/node-test-node-addon-api-new/1241/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1242/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1243/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1244/ ✗

OK, I had some success with Windows 10, but then Ubuntu decided to hang. I think between https://ci.nodejs.org/job/node-test-node-addon-api-new/1240/, which demonstrates a successful run on Windows, and all the other jobs, which demonstrate a successful run on all other platforms, IMO we have a successful run.

gabrielschulhof pushed a commit that referenced this pull request Dec 5, 2019
@dmitryash
objectwrap: add template methods
ce91e14 
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@gabrielschulhof
Copy link
Contributor

Landed in ce91e14.

@JckXia JckXia mentioned this pull request May 9, 2021
Closed
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
objectwrap: add template methods
441e44c 
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
objectwrap: add template methods
5b3b2ef 
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
objectwrap: add template methods
bad1872 
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
objectwrap: add template methods
d9e4ae9 
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@NickNaso NickNaso Awaiting requested review from NickNaso

@mhdawson mhdawson Awaiting requested review from mhdawson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid extra memory allocation with templates
4 participants
@dmitryash @gabrielschulhof @legendecas @mhdawson