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

lib,src: use built-in array buffer detach, transfer #54837

Merged
merged 1 commit into from
Sep 16, 2024
Merged

lib,src: use built-in array buffer detach, transfer #54837

merged 1 commit into from
Sep 16, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 7, 2024

Following up on #50130

@anonrig anonrig added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Sep 7, 2024
@anonrig anonrig requested a review from legendecas September 7, 2024 17:38
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 7, 2024
@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 6723407 to b1e1e32 Compare September 7, 2024 19:02
@codecov Codecov
Copy link

codecov bot commented Sep 7, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (dcc2ed9) to head (ce678e1).
Report is 82 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54837      +/-   ##
==========================================
+ Coverage   87.62%   87.89%   +0.26%     
==========================================
  Files         650      651       +1     
  Lines      182983   183299     +316     
  Branches    35406    35709     +303     
==========================================
+ Hits       160336   161105     +769     
+ Misses      15917    15472     -445     
+ Partials     6730     6722       -8
Files with missing lines Coverage Δ
lib/internal/util.js 96.82% <ø> (+0.06%) ⬆️
lib/internal/webstreams/readablestream.js 98.32% <100.00%> (-0.01%) ⬇️
lib/internal/webstreams/util.js 96.51% <ø> (+3.98%) ⬆️
src/node_util.cc 86.02% <ø> (+0.25%) ⬆️

... and 72 files with indirect coverage changes

@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from b1e1e32 to 6621617 Compare September 7, 2024 22:23
@anonrig anonrig requested a review from legendecas September 7, 2024 22:24
@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 6621617 to 2738f00 Compare September 8, 2024 14:58
@anonrig
Copy link
Member Author

anonrig commented Sep 8, 2024

@nodejs/primordials how do you add a primordial for an attribute? ArrayBufferPrototypeDetached is an attribute not a function. Ex: buffer.detached

@ljharb
Copy link
Member

ljharb commented Sep 8, 2024

It’s a getter function on the prototype - uncurryThis(Object.getOwnPropertyDescriptor(ArrayBuffer.prototype, 'detached').get) should do it.

@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 2738f00 to cbe6bf7 Compare September 8, 2024 15:04
@anonrig
Copy link
Member Author

anonrig commented Sep 8, 2024

It’s a getter function on the prototype - uncurryThis(Object.getOwnPropertyDescriptor(ArrayBuffer.prototype, 'detached').get) should do it.

Thank you @ljharb. I updated primordials.js, would you mind checking it?

@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from cbe6bf7 to 646680d Compare September 8, 2024 15:20
ljharb
ljharb approved these changes Sep 8, 2024
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2024
@anonrig
Copy link
Member Author

anonrig commented Sep 9, 2024

@jasnell it seems ci is down. 8 hours and counting for picking up this task...

@anonrig anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 646680d to ce678e1 Compare September 9, 2024 14:41
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@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/62307/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2024
@nodejs-github-bot nodejs-github-bot merged commit 9195210 into nodejs:main Sep 16, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9195210

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@anonrig @RafaelGSS
lib,src: use built-in array buffer detach, transfer
dde23d0 
PR-URL: #54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@anonrig @RafaelGSS
lib,src: use built-in array buffer detach, transfer
856d59e 
PR-URL: #54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@anonrig @RafaelGSS
lib,src: use built-in array buffer detach, transfer
7415ec7 
PR-URL: #54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@anonrig @RafaelGSS
lib,src: use built-in array buffer detach, transfer
b59c8b8 
PR-URL: #54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@anonrig @louwers
lib,src: use built-in array buffer detach, transfer
40490dd 
PR-URL: nodejs#54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@anonrig @tpoisseau
lib,src: use built-in array buffer detach, transfer
9bb2fbd 
PR-URL: nodejs#54837
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@legendecas legendecas legendecas approved these changes

@jasnell jasnell jasnell approved these changes

@Qard Qard Qard approved these changes

@ljharb ljharb ljharb approved these changes

@richardlau richardlau Awaiting requested review from richardlau

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anonrig @nodejs-github-bot @ljharb @Qard @jasnell @legendecas @aduh95