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

object: add templated property descriptors #610

Closed
wants to merge 1 commit into from
Closed

object: add templated property descriptors #610

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

No description provided.

@gabrielschulhof gabrielschulhof mentioned this pull request Nov 27, 2019
Closed
@gabrielschulhof gabrielschulhof force-pushed the templated-property-descriptors branch from 8ec7f34 to 70fac85 Compare November 29, 2019 07:17
@gabrielschulhof gabrielschulhof changed the title WIP: object: add templated property descriptors object: add templated property descriptors Dec 3, 2019
NickNaso
NickNaso reviewed Dec 3, 2019
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Just some comments about documentation.

doc/property_descriptor.md Outdated Show resolved Hide resolved
doc/property_descriptor.md Show resolved Hide resolved
doc/property_descriptor.md Show resolved Hide resolved
@gabrielschulhof gabrielschulhof force-pushed the templated-property-descriptors branch from 70fac85 to 6e45426 Compare December 3, 2019 22:23
NickNaso
NickNaso approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member

@NickNaso NickNaso 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 Author

gabrielschulhof commented Dec 4, 2019 •
edited
Loading

CI:

Version Job Status
v8.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1216/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1219/ ✗

@gabrielschulhof gabrielschulhof force-pushed the templated-property-descriptors branch from 6e45426 to 899a12a Compare December 4, 2019 17:23
object: add templated property descriptors
189176b 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.
@gabrielschulhof gabrielschulhof force-pushed the templated-property-descriptors branch from 66a07c1 to 189176b Compare December 4, 2019 22:50
@gabrielschulhof
Copy link
Contributor Author

Fixed Windows compilation issues.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Dec 4, 2019 •
edited
Loading

New CI:

Version Job Status
v8.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1221/ ✗

Looks like there is a Win10 issue.

@gabrielschulhof
Copy link
Contributor Author

Trying a build against v14.x: https://ci.nodejs.org/job/node-test-node-addon-api-new/1222/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Dec 5, 2019 •
edited
Loading

OK, since that seemed to work, let's see how low we can go 🙂

Version Job status
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1222/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1223/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1224/ ✓
v8.x https://ci.nodejs.org/job/node-test-node-addon-api-new/1225/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1226/ ✗
https://ci.nodejs.org/job/node-test-node-addon-api-new/1233/ ✓

Windows 10 is still broken for v8.x.

gabrielschulhof pushed a commit that referenced this pull request Dec 5, 2019
object: add templated property descriptors
cfa71b6 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.

PR-URL: #610
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in cfa71b6.

@gabrielschulhof gabrielschulhof deleted the templated-property-descriptors branch December 17, 2019 01:36
@JckXia JckXia mentioned this pull request Apr 30, 2021
Closed
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
object: add templated property descriptors
4fe4ac7 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.

PR-URL: nodejs/node-addon-api#610
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Tobias Nießen <[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
object: add templated property descriptors
1c5f989 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.

PR-URL: nodejs/node-addon-api#610
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Tobias Nießen <[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
object: add templated property descriptors
5c7b75f 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.

PR-URL: nodejs/node-addon-api#610
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Tobias Nießen <[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
object: add templated property descriptors
345731c 
Add static methods to `PropertyDescriptor` that allows the definition
of accessors where the getter/setter is specified as a template
parameter rather than a function parameter. This allows us to avoid
heap-allocating callback data.

PR-URL: nodejs/node-addon-api#610
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Tobias Nießen <[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

@tniessen tniessen tniessen approved these changes

@NickNaso NickNaso NickNaso approved these changes

@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.
4 participants
@gabrielschulhof @NickNaso @tniessen @legendecas