Repository navigation
More (unnecessary?) NAPI_PREAMBLE() calls #238
Description
We have
-
napi_is_array(val->IsArray()) -
napi_is_buffer(node::Buffer::HasInstance()) -
napi_is_arraybuffer(val->IsArrayBuffer()) -
napi_is_typedarray(val->IsTypedArray())
that have NAPI_PREAMBLE(), whereas
-
napi_is_error(val->IsNativeError())
does not have it.
I don't believe the ones above that have NAPI_PREAMBLE() need it, for the same reason that napi_typeof() doesn't need it. What do @nodejs/addon-api think?
Metadata
Metadata
Assignees
Labels
No labels
Activity
mhdawson commentedon Apr 27, 2017
If you have checked and don't believe we need them there, I'm happy to have them be removed.
gabrielschulhof commentedon Apr 27, 2017
The only thing I'm not sure about is whether the engine would ever throw an exception in response to
IsArrayBuffer(), orIsArray()or functions of this nature. I think I'll remove theNAPI_PREAMBLE()s, because it doesn't make sense to me that it should ever throw an exception in such cases.jasongin commentedon Apr 27, 2017
+1
The four listed above seem the most obvious, though we may also consider several other APIs that don't have any possibility of invoking JavaScript code. Does V8 ever throw exceptions when not invoking JS code? I assume not.
gabrielschulhof commentedon Apr 27, 2017
OK, so while we're on the topic, what about these:
napi_get_value_string_latin1napi_get_value_string_utf8napi_get_value_string_utf16napi_get_value_externalnapi_get_buffer_infonapi_get_arraybuffer_infonapi_get_typedarray_infoWe should test how these functions behave if we pass in the wrong type, or, better yet
CHECK_TO_<type>at the top.jasongin commentedon Apr 27, 2017
Generally we have avoided automatic type coercions, because they can have a significant perf cost. (I think there may even be a few places currently doing type coercions that shouldn't.) If the wrong type is passed into an API, it should just return an appropriate error code.
jasongin commentedon Apr 27, 2017
And yes, I agree with adding all those getters to the list.
gabrielschulhof commentedon Apr 27, 2017
They all have checks, so that's good.
gabrielschulhof commentedon Apr 27, 2017
Yeah, sorry, I meant type checks, not coercions.
n-api: remove unnecessary try-catch bracket from certain APIs
n-api: remove unnecessary try-catch bracket from certain APIs
n-api: remove unnecessary try-catch bracket from certain APIs
n-api: remove unnecessary try-catch bracket from certain APIs
n-api: remove unnecessary try-catch bracket from certain APIs