More (unnecessary?) NAPI_PREAMBLE() calls #238
Comments
|
If you have checked and don't believe we need them there, I'm happy to have them be removed. |
|
The only thing I'm not sure about is whether the engine would ever throw an exception in response to |
|
+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. |
|
OK, so while we're on the topic, what about these:
|
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. |
|
And yes, I agree with adding all those getters to the list. |
|
They all have checks, so that's good. |
|
Yeah, sorry, I meant type checks, not coercions. |
These APIs do not need a try-catch around their body, because no exceptions are thrown in their implementation: - `napi_is_array()` - `napi_get_value_string_latin1()` - `napi_get_value_string_utf8()` - `napi_get_value_string_utf16()` - `napi_get_value_external()` - `napi_is_buffer()` - `napi_is_arraybuffer()` - `napi_get_arraybuffer_info()` - `napi_is_typedarray()` - `napi_get_typedarray_info()` Fixes nodejs/abi-stable-node#238
These APIs do not need a try-catch around their body, because no exceptions are thrown in their implementation: - `napi_is_array()` - `napi_get_value_string_latin1()` - `napi_get_value_string_utf8()` - `napi_get_value_string_utf16()` - `napi_get_value_external()` - `napi_is_buffer()` - `napi_is_arraybuffer()` - `napi_get_arraybuffer_info()` - `napi_is_typedarray()` - `napi_get_typedarray_info()` Fixes: nodejs/abi-stable-node#238 PR-URL: nodejs#12705 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
These APIs do not need a try-catch around their body, because no exceptions are thrown in their implementation: - `napi_is_array()` - `napi_get_value_string_latin1()` - `napi_get_value_string_utf8()` - `napi_get_value_string_utf16()` - `napi_get_value_external()` - `napi_is_buffer()` - `napi_is_arraybuffer()` - `napi_get_arraybuffer_info()` - `napi_is_typedarray()` - `napi_get_typedarray_info()` Fixes: nodejs/abi-stable-node#238 PR-URL: nodejs#12705 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
These APIs do not need a try-catch around their body, because no exceptions are thrown in their implementation: - `napi_is_array()` - `napi_get_value_string_latin1()` - `napi_get_value_string_utf8()` - `napi_get_value_string_utf16()` - `napi_get_value_external()` - `napi_is_buffer()` - `napi_is_arraybuffer()` - `napi_get_arraybuffer_info()` - `napi_is_typedarray()` - `napi_get_typedarray_info()` Fixes: nodejs/abi-stable-node#238 Backport-PR-URL: #19447 PR-URL: #12705 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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(), whereasnapi_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 thatnapi_typeof()doesn't need it. What do @nodejs/addon-api think?The text was updated successfully, but these errors were encountered: