Skip to content
Closed
33 changes: 14 additions & 19 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ function fromString(string, encoding) {
return b;
}

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

function fromObject(obj) {
if (obj instanceof Buffer) {
Expand All @@ -135,40 +142,28 @@ function fromObject(obj) {
}

if (Array.isArray(obj)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Even this is not necessary now

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

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


Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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