GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib,src: use built-in array buffer detach, transfer by anonrig · Pull Request #54837 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Sep 7, 2024

Copy link
Copy Markdown
Member

Following up on #50130

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and avivkeller reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@anonrig anonrig added dont-land-on-v18.x labels Sep 7, 2024
@anonrig
anonrig requested a review from legendecas September 7, 2024 17:38
@nodejs-github-bot

nodejs-github-bot commented Sep 7, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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
legendecas
legendecas reviewed Sep 7, 2024
View reviewed changes
Comment thread lib/internal/modules/esm/worker.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@codecov

codecov Bot commented Sep 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

anonrig commented Sep 8, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ljharb

ljharb commented Sep 8, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 2738f00 to cbe6bf7 Compare September 8, 2024 15:04
@anonrig

anonrig commented Sep 8, 2024

Copy link
Copy Markdown
Member Author

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?
ljharb reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig requested review from aduh95, jasnell and richardlau September 8, 2024 15:05
@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

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Sep 8, 2024
View reviewed changes
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 8, 2024
@anonrig

anonrig commented Sep 9, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas approved these changes Sep 9, 2024
View reviewed changes
aduh95
aduh95 reviewed Sep 9, 2024
View reviewed changes
Comment thread lib/internal/per_context/primordials.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Sep 9, 2024
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
lib,src: use built-in array buffer detach, transfer
ce678e1
@anonrig
anonrig force-pushed the yagiz/use-array-buffer-transfer branch from 646680d to ce678e1 Compare September 9, 2024 14:41
aduh95
aduh95 approved these changes Sep 9, 2024
View reviewed changes
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 9, 2024
Qard
Qard approved these changes Sep 9, 2024
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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
Qard
Qard approved these changes Sep 10, 2024
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62307/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 14, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 15, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 16, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 9195210 into nodejs:main Sep 16, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 16, 2024

Copy link
Copy Markdown
Collaborator

Landed in 9195210
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
v22.9.0 proposal #54966
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb approved these changes
@Qard Qard Qard approved these changes
@jasnell jasnell jasnell approved these changes
@legendecas legendecas legendecas approved these changes
@aduh95 aduh95 aduh95 approved these changes
@richardlau richardlau Awaiting requested review from richardlau

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@anonrig @nodejs-github-bot @ljharb @Qard @jasnell @legendecas @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.