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.

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

Implement Buffer.copyBytesFrom #348

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

chjj
Copy link
Contributor

@chjj chjj commented Feb 3, 2024

Implements Buffer.copyBytesFrom.

Also fixes a bug with ERR_INVALID_ARG_TYPE.

Comment on lines +258 to +280
if (offset !== undefined || length !== undefined) {
if (offset !== undefined) {
validateInteger(offset, 'offset')
if (offset >= view.length) return createBuffer(0)
} else {
offset = 0
}

let end

if (length !== undefined) {
validateInteger(length, 'length')
end = offset + length
} else {
end = view.length
}

view = view.subarray(offset, end)
}

view = new Uint8Array(view.buffer, view.byteOffset, view.byteLength)

return fromArrayView(view)
Copy link
Collaborator

@dcousens dcousens Feb 11, 2024

Choose a reason for hiding this comment

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

Suggested change
if (offset !== undefined || length !== undefined) {
if (offset !== undefined) {
validateInteger(offset, 'offset')
if (offset >= view.length) return createBuffer(0)
} else {
offset = 0
}
let end
if (length !== undefined) {
validateInteger(length, 'length')
end = offset + length
} else {
end = view.length
}
view = view.subarray(offset, end)
}
view = new Uint8Array(view.buffer, view.byteOffset, view.byteLength)
return fromArrayView(view)
if (offset !== undefined || length !== undefined) {
if (offset !== undefined) validateInteger(offset, 'offset')
if (length !== undefined) validateInteger(length, 'length')
const offset_ = offset ?? 0
if (offset_ >= view.length) return createBuffer(0)
const end = offset_ + (length ?? view.length)
view = view.subarray(offset_, end)
}
view = new Uint8Array(view.buffer, view.byteOffset, view.byteLength)
return fromArrayView(view)

Copy link
Collaborator

@dcousens dcousens Feb 11, 2024

Choose a reason for hiding this comment

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

Generally, a preference for flat (preferably functional) style, even if the actual number of resulting branches is the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was ported from the node.js code as faithfully as possible to ensure accurate behavior. I can change it though.

While we're on the subject: what is the minimum ES version we support? Since the ?? operator is an ES2020 feature, it would be good to have clarity on that. BigInt is also an ES2020 feature, but we include graceful degradation for the bigint methods.

Copy link
Collaborator

@dcousens dcousens left a comment

Choose a reason for hiding this comment

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

LGTM, with a nit

u16[0] = 0;
assert.strictEqual(b16.length, 2);
assert.strictEqual(b16[0], 255);
assert.strictEqual(b16[1], 255);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are semi's here? 🤔
Shouldn't the linter break on this?

Copy link
Collaborator

@dcousens dcousens Feb 11, 2024

Choose a reason for hiding this comment

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

(no changes required, we can resolve this in another pull request)

Copy link
Contributor Author

@chjj chjj Mar 1, 2024

Choose a reason for hiding this comment

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

I think it's because I pulled these directly from the node.js tests. Maybe we don't lint the node tests?

Copy link
Collaborator

@dcousens dcousens Mar 1, 2024

Choose a reason for hiding this comment

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

If these are directly from the node tests, can you add an please add a reference?
(and adhere to whatever LICENSE notice may be required if this is a copy)

Copy link
Contributor Author

@chjj chjj Mar 1, 2024

Choose a reason for hiding this comment

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

Is that not already accounted for? As far as I can tell, all of the tests in test/node are ripped straight from the node.js codebase, right down to the filenames. I even recognize these ones -- I wrote them. 😊

edit: Okay, after looking, I guess it isn't accounted for. Should I add a the node.js LICENSE file inside the test/node directory, or should I concatenate the existing LICENSE file with the node.js one?

Copy link
Collaborator

@dcousens dcousens Mar 2, 2024

Choose a reason for hiding this comment

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

Thanks for hunting that down @chjj, everything should be MIT, so hopefully any effort here is simply about proper attribution.
I think an inline URL would be helpful too so others can verify the tests against the upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants