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

Conversation

entertainyou
Copy link
Contributor

No description provided.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 29, 2016
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.

@entertainyou
Copy link
Contributor Author

updated pull request, @mscdex

@mscdex
Copy link
Contributor

mscdex commented Jan 31, 2016

@entertainyou
Copy link
Contributor Author

Ping, how to make this PR go forward?

@mscdex
Copy link
Contributor

mscdex commented Feb 1, 2016

/cc @trevnorris

if (Array.isArray(obj)) {
return fromArrayLike(obj);
}

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 .

@@ -135,40 +142,28 @@ function fromObject(obj) {
}

if (Array.isArray(obj)) {
Copy link
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

@entertainyou
Copy link
Contributor Author

updated according to your comments, @thefourtheye

@@ -151,30 +150,20 @@ function fromObject(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

@@ -28,6 +28,10 @@ var c = new Buffer(512);
console.log('c.length == %d', c.length);
assert.strictEqual(512, c.length);

var d = new Buffer([]);
console.log('d.length == %d', d.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we don't expect successful tests to print anything. Can you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the tc above also prints, should I remove all log statement in this test-buffer.js?

Copy link
Contributor

Choose a reason for hiding this comment

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

That falls outside the scope of this PR :) So I would suggest removing only this.

@thefourtheye
Copy link
Contributor

Okay. LGTM with a nit. But I defer it to @trevnorris to call the shot ;-)

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

LGTM

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. :)

@trevnorris
Copy link
Contributor

One request to add a test, but LGTM.

@jasnell
Copy link
Member

jasnell commented Feb 2, 2016

@entertainyou
Copy link
Contributor Author

@jasnell it's this pr Ready To merge ? I'm new here.

@jasnell
Copy link
Member

jasnell commented Feb 3, 2016

@trevnorris ... can you give it one last look over? LGTM

trevnorris pushed a commit that referenced this pull request Feb 5, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: #4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link
Contributor

Landed in c0bfac6 with added commit message body and removed unnecessary else due to return in previous if. Thanks much!

@trevnorris trevnorris closed this Feb 5, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: #4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 9, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: #4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins
Copy link
Contributor

is this something we would like to backport?

@trevnorris
Copy link
Contributor

Not necessary but also is low/no risk. Though it may make tracking future changes easier.

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

SGTM for LTS

MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: #4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: #4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Add fromArrayLike() to handle logic of copying in values from array-like
argument.

PR-URL: nodejs#4948
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
lpinca referenced this pull request Mar 13, 2018
PR-URL: #19279
Refs: #19275 (comment)
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos referenced this pull request Mar 17, 2018
PR-URL: #19279
Refs: #19275 (comment)
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins referenced this pull request Mar 20, 2018
PR-URL: #19279
Refs: #19275 (comment)
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova referenced this pull request in MayaLekova/node May 8, 2018
PR-URL: nodejs#19279
Refs: nodejs#19275 (comment)
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants