FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

n-api: implement wrapping using private properties by gabrielschulhof · Pull Request #18311 · nodejs/node · GitHub

/ node Public

n-api: implement wrapping using private properties - #18311

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:use-private-for-wrap
ConversationCommits (1)ChecksFiles changed
Closed

n-api: implement wrapping using private properties#18311
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:use-private-for-wrap

Conversation

gabrielschulhof commented Jan 23, 2018

Copy link
Copy Markdown
Contributor

Fixes: #14367

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 23, 2018
gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jan 23, 2018
addaleax approved these changes Jan 28, 2018
View reviewed changes
Comment thread src/node_api.cc Outdated

addaleax Jan 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

env.h has some convenience wrappers for accessing private methods (look for PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES), can we use those?

Comment thread src/node_api.cc Outdated

addaleax Jan 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We generally use snake_case for local variables and parameters

gabrielschulhof force-pushed the use-private-for-wrap branch 3 times, most recently from 2465f08 to b6d330e Compare January 30, 2018 01:06

gabrielschulhof commented Jan 30, 2018

Copy link
Copy Markdown
Contributor Author

@addaleax I have addressed your review comments.

BridgeAR commented Feb 1, 2018

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/12887/

mhdawson approved these changes Feb 1, 2018
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

bnoordhuis reviewed Feb 2, 2018
View reviewed changes
Comment thread src/node_api.cc Outdated

bnoordhuis Feb 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

s/prefix/suffix/?

Comment thread src/node_api.cc Outdated

bnoordhuis Feb 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since you have a reference to the v8::Context, it would be more efficient to call Environment::GetCurrent(context) once and then call env->napi_env() directly.

Applies to a few more spots. I won't point them out individually.

Comment thread src/node_api.cc Outdated

bnoordhuis Feb 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Consider using an enum instead of a bool. More self-descriptive at the call site.

Comment thread src/node_api.cc Outdated

bnoordhuis Feb 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Space before :.

Comment thread src/node_api.cc Outdated

bnoordhuis Feb 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you line up the arguments?

gabrielschulhof commented Feb 2, 2018

Copy link
Copy Markdown
Contributor Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/12914/

gabrielschulhof commented Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

I re-ran the node-test-commit-arm portion, and it passed: https://ci.nodejs.org/job/node-test-commit-arm/13589/

gabrielschulhof commented Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 1286923.

gabrielschulhof pushed a commit that referenced this pull request Feb 3, 2018
n-api: implement wrapping using private properties
1286923 
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
gabrielschulhof deleted the use-private-for-wrap branch February 3, 2018 01:27
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
n-api: implement wrapping using private properties
1385daf 
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
n-api: implement wrapping using private properties
9466558 
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
n-api: implement wrapping using private properties
ae8f5db 
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
MylesBorins mentioned this pull request Feb 21, 2018
Merged

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

gabrielschulhof commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

@MylesBorins this is part of #19265.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
n-api: implement wrapping using private properties
17862cd 
PR-URL: nodejs#18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: nodejs#14367
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
n-api: implement wrapping using private properties
5c183d7 
PR-URL: nodejs#18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: nodejs#14367
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
n-api: implement wrapping using private properties
b565ba2 
Backport-PR-URL: #19447
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
MylesBorins mentioned this pull request Apr 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 1, 2018
n-api: implement wrapping using private properties
d89f593 
Backport-PR-URL: #19265
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
n-api: implement wrapping using private properties
18409c5 
PR-URL: nodejs#18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: nodejs#14367
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis left review comments
addaleax addaleax approved these changes
TimothyGu Awaiting requested review from TimothyGu
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using v8::Private::ForApi for N-API object wrapping

8 participants

Back | FazBrowse Home | New Git URL