FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Added napi_is_detached_arraybuffer method by alexahdp · Pull Request #30317 · nodejs/node · GitHub

/ node Public

Added napi_is_detached_arraybuffer method - #30317

Closed
alexahdp wants to merge 1 commit into
nodejs:masterfrom
alexahdp:issue-29955
ConversationCommits (1)ChecksFiles changed
Closed

Added napi_is_detached_arraybuffer method#30317
alexahdp wants to merge 1 commit into
nodejs:masterfrom
alexahdp:issue-29955

Conversation

alexahdp commented Nov 6, 2019

Copy link
Copy Markdown
Contributor
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows [commit guidelines]

Fixes: #29955

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 6, 2019
devnexen reviewed Nov 6, 2019
View reviewed changes
Comment thread src/js_native_api_v8.cc
}

if (!val->IsArrayBuffer()) {
*result = v8impl::JsValueFromV8LocalValue(v8::False(isolate));

devnexen Nov 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Overall result setting might be doable with less lines.

BridgeAR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2019
legendecas reviewed Nov 7, 2019
View reviewed changes
Comment thread src/js_native_api_v8.cc
v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);

if (!val->IsObject()) {
napi_throw_type_error(env,

legendecas Nov 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No JavaScript exceptions were expected on N-API checks. See discussion at #29849.

devsnek Nov 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You could just remove this check and let the below one handle returning false.

legendecas added the node-api Issues and PRs related to the Node-API. label Nov 7, 2019
mhdawson reviewed Nov 10, 2019
View reviewed changes
Comment thread src/js_native_api.h
napi_value* result);
NAPI_EXTERN napi_status napi_is_detached_arraybuffer(napi_env env,
napi_value value,
bool* result);

mhdawson Nov 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This needs to go in the #ifdef NAPI_EXPERIMENTAL section to start.

gabrielschulhof reviewed Nov 11, 2019
View reviewed changes

gabrielschulhof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Aside from the existing comments, the PR also needs tests and documentation.

Comment thread src/js_native_api_v8.cc
napi_status napi_is_detached_arraybuffer(napi_env env,
napi_value value,
napi_value* result) {
NAPI_PREAMBLE(env);

gabrielschulhof Nov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think we need NAPI_PREAMBLE() here, because we don't have any Maybes.

lundibundi reviewed Nov 12, 2019
View reviewed changes
Comment thread src/js_native_api_v8.cc
}

return GET_RETURN_STATUS(env);
}

lundibundi Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

addaleax reviewed Nov 12, 2019
View reviewed changes
Comment thread src/js_native_api_v8.cc
return GET_RETURN_STATUS(env);
}

v8::Local<v8::ArrayBuffer> buffer = v8::Local<v8::ArrayBuffer>::Cast(val);

addaleax Nov 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
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)

Comment thread src/js_native_api_v8.cc

v8::Local<v8::ArrayBuffer> buffer = v8::Local<v8::ArrayBuffer>::Cast(val);

if (buffer->GetContents().Data() != nullptr) {

addaleax Nov 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott commented Nov 23, 2019

Copy link
Copy Markdown
Member

@alexahdp Are you able to add changes to address the comments?

alexahdp commented Nov 23, 2019

Copy link
Copy Markdown
Contributor Author

I don't think so. I'll close this PR.

alexahdp closed this Nov 23, 2019
lundibundi mentioned this pull request Nov 23, 2019
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
gabrielschulhof gabrielschulhof left review comments
legendecas legendecas left review comments
+4 more reviewers
devnexen devnexen left review comments
devsnek devsnek left review comments
lundibundi lundibundi left review comments
mhdawson mhdawson left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

n-api: add napi_is_detached_arraybuffer()
Back | FazBrowse Home | New Git URL