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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 44 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,41 @@ Buffer.allocUnsafeSlow = function (size) {
return allocUnsafe(size)
}

/**
* Copies the underlying memory of `view` into a new `Buffer`.
*/
Buffer.copyBytesFrom = function copyBytesFrom (view, offset, length) {
if (!ArrayBuffer.isView(view) || !view.subarray) {
throw new errors.ERR_INVALID_ARG_TYPE('view', 'TypedArray', view)
}

if (view.length === 0) return createBuffer(0)

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)
Comment on lines +258 to +280
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.

}

function fromString (string, encoding) {
if (typeof encoding !== 'string' || encoding === '') {
encoding = 'utf8'
Expand Down Expand Up @@ -1878,8 +1913,8 @@ E('ERR_BUFFER_OUT_OF_BOUNDS',
return 'Attempt to access memory outside buffer bounds'
}, RangeError)
E('ERR_INVALID_ARG_TYPE',
function (name, actual) {
return `The "${name}" argument must be of type number. Received type ${typeof actual}`
function (name, type, actual) {
return `The "${name}" argument must be of type ${type}. Received type ${typeof actual}`
}, TypeError)
E('ERR_OUT_OF_RANGE',
function (str, range, input) {
Expand Down Expand Up @@ -1943,6 +1978,13 @@ function validateNumber (value, name) {
}
}

function validateInteger (value, name) {
validateNumber(value, name)
if ((value >>> 0) !== value) {
throw new errors.ERR_BUFFER_OUT_OF_BOUNDS(name)
}
}

function boundsError (value, length, type) {
if (Math.floor(value) !== value) {
validateNumber(value, type)
Expand Down
73 changes: 72 additions & 1 deletion test/node/test-buffer-from.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
var Buffer = require('../../').Buffer;

const common = require('./common');
const { deepStrictEqual, throws } = require('assert');
const assert = require('assert');
const { deepStrictEqual, throws } = assert;
const { runInNewContext } = require('vm');

const checkString = 'test';
Expand Down Expand Up @@ -66,3 +67,73 @@ deepStrictEqual(
throws(() => Buffer.from(input), errMsg);
});

{
const u16 = new Uint16Array([0xffff]);
const b16 = Buffer.copyBytesFrom(u16);
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

}

{
const u16 = new Uint16Array([0, 0xffff]);
const b16 = Buffer.copyBytesFrom(u16, 1, 5);
u16[0] = 0xffff;
u16[1] = 0;
assert.strictEqual(b16.length, 2);
assert.strictEqual(b16[0], 255);
assert.strictEqual(b16[1], 255);
}

{
const u32 = new Uint32Array([0xffffffff]);
const b32 = Buffer.copyBytesFrom(u32);
u32[0] = 0;
assert.strictEqual(b32.length, 4);
assert.strictEqual(b32[0], 255);
assert.strictEqual(b32[1], 255);
assert.strictEqual(b32[2], 255);
assert.strictEqual(b32[3], 255);
}

assert.throws(() => {
Buffer.copyBytesFrom();
}, TypeError);

{
const dv = new DataView(new ArrayBuffer(1));
assert.throws(() => {
Buffer.copyBytesFrom(dv);
}, TypeError);
}

['', Symbol(), true, false, {}, [], () => {}, 1, 1n, null, undefined].forEach(
notTypedArray => assert.throws(() => {
Buffer.copyBytesFrom(notTypedArray);
}, TypeError)
);

['', Symbol(), true, false, {}, [], () => {}, 1n].forEach(notANumber =>
assert.throws(() => {
Buffer.copyBytesFrom(new Uint8Array(1), notANumber);
}, TypeError)
);

[-1, NaN, 1.1, -Infinity].forEach(outOfRange =>
assert.throws(() => {
Buffer.copyBytesFrom(new Uint8Array(1), outOfRange);
}, RangeError)
);

['', Symbol(), true, false, {}, [], () => {}, 1n].forEach(notANumber =>
assert.throws(() => {
Buffer.copyBytesFrom(new Uint8Array(1), 0, notANumber);
}, TypeError)
);

[-1, NaN, 1.1, -Infinity].forEach(outOfRange =>
assert.throws(() => {
Buffer.copyBytesFrom(new Uint8Array(1), 0, outOfRange);
}, RangeError)
);