buffer: refactor to remove some duplicated code in fromObject. by entertainyou · Pull Request #4948 · nodejs/node · GitHub

No description provided.

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.

updated pull request, @mscdex

CI: https://ci.nodejs.org/job/node-test-pull-request/1454/

Ping, how to make this PR go forward?

/cc @trevnorris

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

updated, @thefourtheye .

Even this is not necessary now

…Object.

updated according to your comments, @thefourtheye

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.

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?

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

…in fromObject.

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

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

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

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

…d code in fromObject.

LGTM

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

One request to add a test, but LGTM.

…plicated code in fromObject.

CI: https://ci.nodejs.org/job/node-test-pull-request/1508/

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

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

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>

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

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>

is this something we would like to backport?

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

SGTM for LTS

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>

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>

Refs: nodejs#19275 (comment)

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>

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>

Refs: #19275 (comment)

buffer: refactor to remove some duplicated code in fromObject.

88f2a9f

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 29, 2016

mscdex reviewed Jan 29, 2016
View reviewed changes

fixup! buffer: refactor to remove some duplicated code in fromObject.

cdaa4ea

thefourtheye reviewed Feb 1, 2016
View reviewed changes

fixup! buffer: refactor to remove some duplicated code in fromObject.

c5a886b

thefourtheye reviewed Feb 1, 2016
View reviewed changes

entertainyou added 2 commits February 1, 2016 16:52

fixup! buffer: refactor to remove some duplicated code in fromObject.

e97323d

fixup! fixup! buffer: refactor to remove some duplicated code in from…

980f208

…Object.

thefourtheye reviewed Feb 1, 2016
View reviewed changes

fixup! fixup! fixup! buffer: refactor to remove some duplicated code …

ab15735

…in fromObject.

thefourtheye reviewed Feb 1, 2016
View reviewed changes

fixup! fixup! fixup! fixup! buffer: refactor to remove some duplicate…

e16e88b

…d code in fromObject.

trevnorris reviewed Feb 1, 2016
View reviewed changes

fixup! fixup! fixup! fixup! fixup! buffer: refactor to remove some du…

b274fe1

…plicated code in fromObject.

trevnorris closed this Feb 5, 2016

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the lts-watch-v4.x label Mar 10, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 17, 2016

ryzokuken mentioned this pull request Mar 6, 2018

test: rename regression tests with descriptive file names #19161

Closed

8 tasks

ryzokuken mentioned this pull request Mar 10, 2018

test: rename regression tests with descriptive file names (pt. 3) #19275

Closed

6 tasks

ryzokuken referenced this pull request in ryzokuken/node Mar 11, 2018

test: delete test/parallel/test-regress-GH-4948

8f3a863

Refs: nodejs#19275 (comment)

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

BethGriggs referenced this pull request Oct 16, 2018

test: delete test/parallel/test-regress-GH-4948

fe3836a

Refs: #19275 (comment)

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

buffer: refactor to remove some duplicated code in fromObject. by entertainyou · Pull Request #4948 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

buffer: refactor to remove some duplicated code in fromObject.#4948entertainyou wants to merge 8 commits intonodejs:masternodejs/node:masterfrom entertainyou:buffer_refactorentertainyou/node:buffer_refactorCopy head branch name to clipboard

Conversation

entertainyou commented Jan 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

entertainyou commented Jan 31, 2016

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

entertainyou commented Feb 1, 2016

Uh oh!

mscdex commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

entertainyou commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Feb 1, 2016

Uh oh!

jasnell commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Feb 1, 2016

Uh oh!

jasnell commented Feb 2, 2016

Uh oh!

entertainyou commented Feb 3, 2016

Uh oh!

jasnell commented Feb 3, 2016

Uh oh!

trevnorris commented Feb 5, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

trevnorris commented Mar 10, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

buffer: refactor to remove some duplicated code in fromObject.#4948
entertainyou wants to merge 8 commits into
nodejs:masterfrom
entertainyou:buffer_refactor