Added napi_is_detached_arraybuffer method#30317
Added napi_is_detached_arraybuffer method#30317alexahdp wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
the
c++
| } | ||
|
|
||
| if (!val->IsArrayBuffer()) { | ||
| *result = v8impl::JsValueFromV8LocalValue(v8::False(isolate)); |
There was a problem hiding this comment.
Overall result setting might be doable with less lines.
BridgeAR
added
the
code-and-learn
| v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value); | ||
|
|
||
| if (!val->IsObject()) { | ||
| napi_throw_type_error(env, |
There was a problem hiding this comment.
No JavaScript exceptions were expected on N-API checks. See discussion at #29849.
There was a problem hiding this comment.
You could just remove this check and let the below one handle returning false.
| napi_value* result); | ||
| NAPI_EXTERN napi_status napi_is_detached_arraybuffer(napi_env env, | ||
| napi_value value, | ||
| bool* result); |
There was a problem hiding this comment.
This needs to go in the #ifdef NAPI_EXPERIMENTAL section to start.
gabrielschulhof
left a comment
gabrielschulhof
left a comment
There was a problem hiding this comment.
Aside from the existing comments, the PR also needs tests and documentation.
| napi_status napi_is_detached_arraybuffer(napi_env env, | ||
| napi_value value, | ||
| napi_value* result) { | ||
| NAPI_PREAMBLE(env); |
There was a problem hiding this comment.
I don't think we need NAPI_PREAMBLE() here, because we don't have any Maybes.
| } | ||
|
|
||
| return GET_RETURN_STATUS(env); | ||
| } |
There was a problem hiding this comment.
Perhaps IIUC
bool is_detached = val->IsArrayBuffer() &&
val.As<v8::ArrayBuffer>()->GetContents().Data() != nullptr;
auto ctor = is_detached ? v8::True : v8::False;
*result = v8impl::JsValueFromV8LocalValue(ctor(isolate));
return GET_RETURN_STATUS(env);Though maybe simple if/else may be better instead of ternary.
| return GET_RETURN_STATUS(env); | ||
| } | ||
|
|
||
| v8::Local<v8::ArrayBuffer> buffer = v8::Local<v8::ArrayBuffer>::Cast(val); |
There was a problem hiding this comment.
| v8::Local<v8::ArrayBuffer> buffer = v8::Local<v8::ArrayBuffer>::Cast(val); | |
| v8::Local<v8::ArrayBuffer> buffer = val.As<v8::ArrayBuffer>(); |
(It’s not important, just more common this way in our source tree)
|
|
||
| v8::Local<v8::ArrayBuffer> buffer = v8::Local<v8::ArrayBuffer>::Cast(val); | ||
|
|
||
| if (buffer->GetContents().Data() != nullptr) { |
There was a problem hiding this comment.
Is this correct? We can create ArrayBuffers with zero length and nullptr data, and I guess we could consider them effectively detached, but maybe !buffer->IsDetachable() is a better choice?
Also, just fyi, GetContents() is scheduled to be deprecated in favour of GetBackingStore(), see e.g. e66a2ac
|
@alexahdp Are you able to add changes to address the comments? |
|
I don't think so. I'll close this PR. |
11 participants
make -j4 test(UNIX), orvcbuild test(Windows) passesFixes: #29955