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

napi: change napi_callback to return napi_value #12248

Closed
wants to merge 3 commits into from
Closed

napi: change napi_callback to return napi_value #12248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f34553
napi: change napi_callback to return napi_value
boingoing Mar 24, 2017
7d899eb
Respond to review feedback
boingoing Apr 9, 2017
b47c9aa
Fix lint errors
boingoing Apr 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
277 changes: 122 additions & 155 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,70 @@ class napi_env__ {
napi_extended_error_info last_error;
};

#define RETURN_STATUS_IF_FALSE(env, condition, status) \
do { \
if (!(condition)) { \
return napi_set_last_error((env), (status)); \
} \
} while (0)

#define CHECK_ENV(env) \
if ((env) == nullptr) { \
node::FatalError(__func__, "environment(env) must not be null"); \
}

#define CHECK_ARG(env, arg) \
RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)

#define CHECK_MAYBE_EMPTY(env, maybe, status) \
RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))

#define CHECK_MAYBE_NOTHING(env, maybe, status) \
RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))

// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
#define NAPI_PREAMBLE(env) \
CHECK_ENV((env)); \
RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
napi_pending_exception); \
napi_clear_last_error((env)); \
v8impl::TryCatch try_catch((env))

#define CHECK_TO_TYPE(env, type, context, result, src, status) \
do { \
auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
CHECK_MAYBE_EMPTY((env), maybe, (status)); \
(result) = maybe.ToLocalChecked(); \
} while (0)

#define CHECK_TO_OBJECT(env, context, result, src) \
CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)

#define CHECK_TO_STRING(env, context, result, src) \
CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)

#define CHECK_TO_NUMBER(env, context, result, src) \
CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)

#define CHECK_TO_BOOL(env, context, result, src) \
CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
napi_boolean_expected)

#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len) \
do { \
auto str_maybe = v8::String::NewFromUtf8( \
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
result = str_maybe.ToLocalChecked(); \
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 also wrap result in parentheses here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. We probably didn't do that originally because result is the LHS but looks like that works fine so I'll make the change.

} while (0)

#define CHECK_NEW_FROM_UTF8(env, result, str) \
CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)

#define GET_RETURN_STATUS(env) \
(!try_catch.HasCaught() ? napi_ok \
: napi_set_last_error((env), napi_pending_exception))

namespace v8impl {

// convert from n-api property attributes to v8::PropertyAttribute
Expand Down Expand Up @@ -123,6 +187,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
return local;
}

napi_status V8NameFromPropertyDescriptor(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

How about either adding static inline here too (and for helpers in general), or – and I think I would prefer that – turning v8impl into an anonymous namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the benefit of static inline but why do you prefer an anonymous namespace? I think there's some value in clumping together the v8 helpers and naming that clump v8impl (though name could be more descriptive 👍). We might have other namespaces for other clumps of helpers like uvimpl, etc so having a relevant name helps to reason about these groups, no?

Copy link
Member

Choose a reason for hiding this comment

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

@boingoing Having an anon namespace would be nice because none of the symbols for the utilities here need to be exported, like static would indicate too. You could keep the v8impl/uvimpl namespaces and just wrap their entire content in a anon namespace if you like that for readability.

Copy link
Member

Choose a reason for hiding this comment

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

Also, just noticed, if you go with static inline can you re-align the arguments here? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you meant about the namespaces. I inferred your comment earlier to mean that you just preferred an unnamed namespace. I'd prefer to punt that particular change into a later PR since I want to get this and #12250 wrapped up early this week.

Is the argument alignment here good? With static inline at the beginning of the line, it's overall too long to have the arguments on subsequent lines align with the left-paren so I just pushed the next lines over to the right. Could also take some of static inline napi_status onto it's own line or align all the arguments by moving the first onto it's own line. Doesn't seem to be a hard rule about how this works and we're pretty inconsistent about it, in general, so I just picked what looked nice to me. If you feel strongly about this, though, I don't mind changing it. 👍

Copy link
Member

Choose a reason for hiding this comment

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

If you feel strongly about this, though

I don’t, keep anything for later PRs that you want. :)

const napi_property_descriptor* p,
v8::Local<v8::Name>* result) {
if (p->utf8name != nullptr) {
CHECK_NEW_FROM_UTF8(env, *result, p->utf8name);
} else {
v8::Local<v8::Value> property_value =
v8impl::V8LocalValueFromJsValue(p->name);

RETURN_STATUS_IF_FALSE(env, property_value->IsName(), napi_name_expected);
*result = property_value.As<v8::Name>();
}

return napi_ok;
}

// Adapter for napi_finalize callbacks.
class Finalizer {
protected:
Expand Down Expand Up @@ -357,13 +437,19 @@ class CallbackWrapperBase : public CallbackWrapper {
v8::Local<v8::External>::Cast(
_cbdata->GetInternalField(kInternalFieldIndex))->Value());
v8::Isolate* isolate = _cbinfo.GetIsolate();

napi_env env = static_cast<napi_env>(
v8::Local<v8::External>::Cast(
_cbdata->GetInternalField(kEnvIndex))->Value());

// Make sure any errors encountered last time we were in N-API are gone.
napi_clear_last_error(env);
cb(env, cbinfo_wrapper);

napi_value result = cb(env, cbinfo_wrapper);

if (result != nullptr) {
this->SetReturnValue(result);
}

if (!env->last_exception.IsEmpty()) {
isolate->ThrowException(
Expand Down Expand Up @@ -604,75 +690,12 @@ void napi_module_register(napi_module* mod) {
node::node_module_register(nm);
}

#define RETURN_STATUS_IF_FALSE(env, condition, status) \
do { \
if (!(condition)) { \
return napi_set_last_error((env), (status)); \
} \
} while (0)

#define CHECK_ENV(env) \
if ((env) == nullptr) { \
node::FatalError(__func__, "environment(env) must not be null"); \
}

#define CHECK_ARG(env, arg) \
RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)

#define CHECK_MAYBE_EMPTY(env, maybe, status) \
RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))

#define CHECK_MAYBE_NOTHING(env, maybe, status) \
RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))

// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
#define NAPI_PREAMBLE(env) \
CHECK_ENV((env)); \
RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
napi_pending_exception); \
napi_clear_last_error((env)); \
v8impl::TryCatch try_catch((env))

#define CHECK_TO_TYPE(env, type, context, result, src, status) \
do { \
auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
CHECK_MAYBE_EMPTY((env), maybe, (status)); \
(result) = maybe.ToLocalChecked(); \
} while (0)

#define CHECK_TO_OBJECT(env, context, result, src) \
CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)

#define CHECK_TO_STRING(env, context, result, src) \
CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)

#define CHECK_TO_NUMBER(env, context, result, src) \
CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)

#define CHECK_TO_BOOL(env, context, result, src) \
CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
napi_boolean_expected)

#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len) \
do { \
auto str_maybe = v8::String::NewFromUtf8( \
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
result = str_maybe.ToLocalChecked(); \
} while (0)

#define CHECK_NEW_FROM_UTF8(env, result, str) \
CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)

#define GET_RETURN_STATUS(env) \
(!try_catch.HasCaught() ? napi_ok \
: napi_set_last_error((env), napi_pending_exception))

// Warning: Keep in-sync with napi_status enum
const char* error_messages[] = {nullptr,
"Invalid pointer passed as argument",
"An object was expected",
"A string was expected",
"A string or symbol was expected",
"A function was expected",
"A number was expected",
"A boolean was expected",
Expand Down Expand Up @@ -789,10 +812,16 @@ napi_status napi_define_class(napi_env env,
continue;
}

v8::Local<v8::String> property_name;
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
v8::Local<v8::Name> property_name;
napi_status status =
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);

if (status != napi_ok) {
return napi_set_last_error(env, status);
}

v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromDescriptor(p);
v8impl::V8PropertyAttributesFromDescriptor(p);
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? We usually do 4-space indents for statement continuations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks for letting me know about that. I just thought this was an oversight.


// This code is similar to that in napi_define_properties(); the
// difference is it applies to a template instead of an object.
Expand All @@ -809,7 +838,7 @@ napi_status napi_define_class(napi_env env,
attributes);
} else if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)


RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);

Expand All @@ -818,7 +847,6 @@ napi_status napi_define_class(napi_env env,
v8impl::FunctionCallbackWrapper::Invoke,
cbdata,
v8::Signature::New(isolate, tpl));
t->SetClassName(property_name);

tpl->PrototypeTemplate()->Set(property_name, t, attributes);
} else {
Expand Down Expand Up @@ -851,18 +879,6 @@ napi_status napi_define_class(napi_env env,
return GET_RETURN_STATUS(env);
}

napi_status napi_set_return_value(napi_env env,
napi_callback_info cbinfo,
napi_value value) {
NAPI_PREAMBLE(env);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

info->SetReturnValue(value);
return GET_RETURN_STATUS(env);
}

napi_status napi_get_property_names(napi_env env,
napi_value object,
napi_value* result) {
Expand Down Expand Up @@ -1098,8 +1114,13 @@ napi_status napi_define_properties(napi_env env,
for (size_t i = 0; i < property_count; i++) {
const napi_property_descriptor* p = &properties[i];

v8::Local<v8::Name> name;
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
v8::Local<v8::Name> property_name;
napi_status status =
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);

if (status != napi_ok) {
return napi_set_last_error(env, status);
}

v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromDescriptor(p);
Expand All @@ -1113,7 +1134,7 @@ napi_status napi_define_properties(napi_env env,

auto set_maybe = obj->SetAccessor(
context,
name,
property_name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
Expand All @@ -1132,8 +1153,8 @@ napi_status napi_define_properties(napi_env env,
v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);

auto define_maybe =
obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
auto define_maybe = obj->DefineOwnProperty(
context, property_name, t->GetFunction(), attributes);

if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_generic_failure);
Expand All @@ -1142,7 +1163,7 @@ napi_status napi_define_properties(napi_env env,
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);

auto define_maybe =
obj->DefineOwnProperty(context, name, value, attributes);
obj->DefineOwnProperty(context, property_name, value, attributes);

if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_invalid_arg);
Expand Down Expand Up @@ -1435,33 +1456,24 @@ napi_status napi_get_cb_info(
napi_value* this_arg, // [out] Receives the JS 'this' arg for the call
void** data) { // [out] Receives the data pointer for the callback.
CHECK_ENV(env);
CHECK_ARG(env, argc);
CHECK_ARG(env, argv);
CHECK_ARG(env, this_arg);
CHECK_ARG(env, data);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

info->Args(argv, std::min(*argc, info->ArgsLength()));
*argc = info->ArgsLength();
*this_arg = info->This();
*data = info->Data();

return napi_ok;
}

napi_status napi_get_cb_args_length(napi_env env,
napi_callback_info cbinfo,
size_t* result) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
CHECK_ENV(env);
CHECK_ARG(env, result);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
if (argv != nullptr) {
CHECK_ARG(env, argc);
info->Args(argv, std::min(*argc, info->ArgsLength()));
}
if (argc != nullptr) {
*argc = info->ArgsLength();
}
if (this_arg != nullptr) {
*this_arg = info->This();
}
if (data != nullptr) {
*data = info->Data();
}

*result = info->ArgsLength();
return napi_ok;
}

Expand All @@ -1479,51 +1491,6 @@ napi_status napi_is_construct_call(napi_env env,
return napi_ok;
}

// copy encoded arguments into provided buffer or return direct pointer to
// encoded arguments array?
napi_status napi_get_cb_args(napi_env env,
napi_callback_info cbinfo,
napi_value* buf,
size_t bufsize) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
CHECK_ENV(env);
CHECK_ARG(env, buf);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

info->Args(buf, bufsize);
return napi_ok;
}

napi_status napi_get_cb_this(napi_env env,
napi_callback_info cbinfo,
napi_value* result) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
CHECK_ENV(env);
CHECK_ARG(env, result);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

*result = info->This();
return napi_ok;
}

napi_status napi_get_cb_data(napi_env env,
napi_callback_info cbinfo,
void** result) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
CHECK_ENV(env);
CHECK_ARG(env, result);

v8impl::CallbackWrapper* info =
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);

*result = info->Data();
return napi_ok;
}

napi_status napi_call_function(napi_env env,
napi_value recv,
napi_value func,
Expand Down
Loading