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

src,node-api: update napi_is_detached_arraybuffer using ArrayBuffer::WasDetached #45538

Merged
merged 1 commit into from
Nov 22, 2022
Merged

src,node-api: update napi_is_detached_arraybuffer using ArrayBuffer::WasDetached #45538

merged 1 commit into from
Nov 22, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Nov 20, 2022

This updates napi_is_detached_arraybuffer(...) using the new API below which looks more straightforward.

node/deps/v8/include/v8-array-buffer.h

Lines 243 to 246 in db88483

/**
* Returns true if this ArrayBuffer has been detached.
*/
bool WasDetached() const;

Signed-off-by: Daeyeon Jeong [email protected]

@daeyeon
src,node-api: update napi_is_detached_arraybuffer
6cdec97 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Nov 20, 2022
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48048/

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2022
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 21, 2022
mhdawson
mhdawson approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

@daeyeon do you know what version of Node.js contain versions of V8 that support the new API? Just thinking about how we tag in terms of backports.

@github-actions github-actions bot mentioned this pull request Nov 22, 2022
Open
15 tasks
@anonrig
Copy link
Member

anonrig commented Nov 22, 2022

@daeyeon do you know what version of Node.js contain versions of V8 that support the new API? Just thinking about how we tag in terms of backports.

v8 10.9.111 adds this function. Referencing commit v8/v8@9df5ef7

@daeyeon
Copy link
Member Author

daeyeon commented Nov 22, 2022

Only the current main (v20-pre) contains the new API and it's added through this patch, 5b8b921. I think its backporting would be possible since the API depends on only v8's internal JSArrayBuffer::was_detached which can be found on Node v14.x also.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit be049df into nodejs:main Nov 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in be049df

@daeyeon daeyeon deleted the 221120.Sun.66e9.main.napi-detached-ab branch November 22, 2022 10:45
@github-actions github-actions bot mentioned this pull request Nov 23, 2022
Open
13 tasks
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
@daeyeon @marco-ippolito
src,node-api: update napi_is_detached_arraybuffer
8c71094 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@github-actions github-actions bot mentioned this pull request Nov 24, 2022
Open
16 tasks
ruyadorno pushed a commit that referenced this pull request Nov 24, 2022
@daeyeon @ruyadorno
src,node-api: update napi_is_detached_arraybuffer
42507e6 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@daeyeon @danielleadams
src,node-api: update napi_is_detached_arraybuffer
667c161 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@daeyeon @danielleadams
src,node-api: update napi_is_detached_arraybuffer
f7dc431 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@daeyeon @danielleadams
src,node-api: update napi_is_detached_arraybuffer
c5630e2 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@daeyeon @danielleadams
src,node-api: update napi_is_detached_arraybuffer
c8bbeb3 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@daeyeon @danielleadams
src,node-api: update napi_is_detached_arraybuffer
56c26c7 
This gets `napi_is_detached_arraybuffer(..)` to use the new API,
`ArrayBuffer::WasDetached`, that looks more straightforward.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45538
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. 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.
5 participants
@daeyeon @nodejs-github-bot @mhdawson @anonrig @legendecas