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

buffer: refactor to remove some duplicated code in fromObject. #4948

Closed
wants to merge 8 commits into from
29 changes: 12 additions & 17 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,41 +134,36 @@ function fromObject(obj) {
return b;
}

if (Array.isArray(obj)) {
if (obj == null) {
throw new TypeError('Must start with number, buffer, array or string');
}

function fromArrayLike(obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be beneficial (performance-wise) to move this function out of fromObject() since it does not depend on any outside variables.

const length = obj.length;
const b = allocate(length);
for (let i = 0; i < length; i++)
b[i] = obj[i] & 255;
return b;
}

if (obj == null) {
throw new TypeError('Must start with number, buffer, array or string');
if (Array.isArray(obj)) {
return fromArrayLike(obj);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline

if (obj instanceof ArrayBuffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this check first, we can skip the isArray check and let the if block below this, take care of the creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, @thefourtheye .

return binding.createFromArrayBuffer(obj);
}

if (obj.buffer instanceof ArrayBuffer || obj.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. We are missing a corner case here. If the length of the array is zero we will throw an error. Perhaps we can change this to 'length' in obj.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about performance impact though. That said, if our tests didn't catch this case, may I ask you to update the test with empty array case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very familiar with node/js internal, but I think the in operator should not be very slow compare to Array.isArray?

PR updated according to your comments

let length;
if (typeof obj.length !== 'number' || obj.length !== obj.length)
length = 0;
else
length = obj.length;
const b = allocate(length);
for (let i = 0; i < length; i++) {
b[i] = obj[i] & 255;
if (typeof obj.length !== 'number' || obj.length !== obj.length) {
return allocate(0);
} else {
return fromArrayLike(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

just an aside, all typed arrays could be more quickly handled in C++, but that doesn't concern this PR. at least now it's centralized and will be easier to make the change. :)

}
return b;
}

if (obj.type === 'Buffer' && Array.isArray(obj.data)) {
var array = obj.data;
const b = allocate(array.length);
for (let i = 0; i < array.length; i++)
b[i] = array[i] & 255;
return b;
return fromArrayLike(obj.data);
}

throw new TypeError('Must start with number, buffer, array or string');
Expand Down