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

n-api: implement wrapping using private properties #18311

Closed

Conversation

gabrielschulhof
Copy link
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 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 gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Jan 23, 2018
src/node_api.cc Outdated
v8::String::NewFromOneByte(isolate,
reinterpret_cast<const uint8_t*>(key),
v8::NewStringType::kInternalized).ToLocalChecked());
}
Copy link
Member

Choose a reason for hiding this comment

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

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

src/node_api.cc Outdated
@@ -815,24 +791,38 @@ static
napi_status Unwrap(napi_env env,
napi_value js_object,
void** result,
v8::Local<v8::Object>* wrapper,
v8::Local<v8::Object>* parent = nullptr) {
bool removeWrap) {
Copy link
Member

Choose a reason for hiding this comment

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

We generally use snake_case for local variables and parameters

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

@addaleax I have addressed your review comments.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

src/node_api.cc Outdated
@@ -46,6 +47,9 @@ struct napi_env__ {
uv_loop_t* loop = nullptr;
};

#define NAPI_PRIVATE_KEY(isolate, prefix) \
(node::Environment::GetCurrent((isolate))->napi_ ## prefix())
Copy link
Member

Choose a reason for hiding this comment

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

s/prefix/suffix/?

src/node_api.cc Outdated
reinterpret_cast<const uint8_t*>("N-API Environment"),
v8::NewStringType::kInternalized).ToLocalChecked());
auto value = global->GetPrivate(context, key).ToLocalChecked();
auto value = global->GetPrivate(context, NAPI_PRIVATE_KEY(isolate, env))
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/node_api.cc Outdated
@@ -815,24 +782,37 @@ static
napi_status Unwrap(napi_env env,
napi_value js_object,
void** result,
v8::Local<v8::Object>* wrapper,
v8::Local<v8::Object>* parent = nullptr) {
bool remove_wrap) {
Copy link
Member

Choose a reason for hiding this comment

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

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

src/node_api.cc Outdated
} else {
// Create a self-deleting reference.
reference = v8impl::Reference::New(env, obj, 0, true, finalize_cb,
native_object, finalize_cb == nullptr ? nullptr: finalize_hint);
Copy link
Member

Choose a reason for hiding this comment

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

Space before :.

src/node_api.cc Outdated
wrapper->SetInternalField(2, v8::External::New(isolate, reference));
}
CHECK(obj->SetPrivate(context, NAPI_PRIVATE_KEY(isolate, wrapper),
v8::External::New(isolate, reference)).FromJust());
Copy link
Member

Choose a reason for hiding this comment

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

Can you line up the arguments?

@gabrielschulhof
Copy link
Contributor Author

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

Landed in 1286923.

gabrielschulhof pushed a commit that referenced this pull request Feb 3, 2018
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
@gabrielschulhof 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
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
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
PR-URL: #18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: #14367
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
@MylesBorins
Copy link
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
Copy link
Contributor Author

@MylesBorins this is part of #19265.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
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
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
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 MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18311
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: nodejs#14367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

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