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:masterfrom
Conversation
lib/buffer.js
Outdated
There was a problem hiding this comment.
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 |
|
Ping, how to make this PR go forward? |
|
/cc @trevnorris |
lib/buffer.js
Outdated
There was a problem hiding this comment.
If we do this check first, we can skip the isArray check and let the if block below this, take care of the creation.
lib/buffer.js
Outdated
There was a problem hiding this comment.
Even this is not necessary now
|
updated according to your comments, @thefourtheye |
lib/buffer.js
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
test/parallel/test-buffer.js
Outdated
There was a problem hiding this comment.
Normally we don't expect successful tests to print anything. Can you remove this?
There was a problem hiding this comment.
the tc above also prints, should I remove all log statement in this test-buffer.js?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
|
@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 |
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: #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: #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>
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: #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: #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>
6 participants
No description provided.