Skip to content

stream: update todo message for detached buffers - #45464

Closed
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:feat/array-buffer-detachable
Closed

stream: update todo message for detached buffers#45464
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:feat/array-buffer-detachable

Conversation

@anonrig

Copy link
Copy Markdown
Member

No description provided.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Nov 14, 2022
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

function isDetachedBuffer(buffer) {
if (ArrayBufferGetByteLength(buffer) === 0) {
// TODO(daeyeon): Consider using C++ builtin to improve performance.
// Replace this when current v8 is updated to include the following commit:

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.

Suggested change
// Replace this when current v8 is updated to include the following commit:
// Replace this when current V8 is updated to include the following commit:

@Trott

Copy link
Copy Markdown
Member

LGTM, but you'll need to rebase against the main branch to pick up the fix for the linting problems.

if (ArrayBufferGetByteLength(buffer) === 0) {
// TODO(daeyeon): Consider using C++ builtin to improve performance.
// Replace this when current v8 is updated to include the following commit:
// https://chromium.googlesource.com/v8/v8/+/9df5ef70ff18977b157028fc55ced5af4bcee535

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.

This commit seems trivial, why don't we just backport it now?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, that's better. I've opened #45474, and closing this pull request.

@anonrig

Copy link
Copy Markdown
MemberAuthor

Closing this due to PR #45512

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@anonrig@nodejs-github-bot@Trott@lpinca@targos