buffer: fix value check for writeUInt{B,L}E#3500
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lib/buffer.js Outdated
There was a problem hiding this comment.
I think Crankshaft and TurboFan are smart enough to move the Math.pow() call to the use site but the baseline compiler probably isn't, which would mean an extra Math.pow() call in the (unoptimized) common case.
There was a problem hiding this comment.
oh whoops. I just moved it b/c it didn't fit in 80 chars, but brain farted that i moved it outside the if. will fix.
jasnell commented Oct 26, 2015
@trevnorris ... would you want this in v4.x? |
trevnorris commented Oct 26, 2015
@jasnell bug fix, so I think so. |
trevnorrisforce-pushed the buf-fix-uint-write branch from 3d2fd99 to 425d681Comparetrevnorris commented Oct 26, 2015
@bnoordhuis Addressed comments. New CI: https://ci.nodejs.org/job/node-test-pull-request/620/ |
trevnorris commented Oct 26, 2015
Two unrelated failures. Everything else looks good. |
trevnorrisforce-pushed the buf-fix-uint-write branch 2 times, most recently from 540da8d to 67d4e96Compare
bnoordhuis commented Oct 26, 2015
LGTM. Consider adding tests for byteLength > 1. |
trevnorris commented Oct 26, 2015
@bnoordhuis do these (https://github.com/nodejs/node/pull/3500/files#diff-aa771a8dc1afdde910c49517a74a0ddeR143) not cover that? |
bnoordhuis commented Oct 26, 2015
I only see tests for byteLength == 0 and 1 or am I missing something? |
trevnorrisforce-pushed the buf-fix-uint-write branch from 67d4e96 to 559e66bCompareFixes: #3497 PR-URL: #3500 Reviewed-By: Ben Noordhuis <[email protected]>
trevnorris commented Oct 26, 2015
Thanks for the review. Landed in 3308e5e. |
Fixes: #3497 PR-URL: #3500 Reviewed-By: Ben Noordhuis <[email protected]>
Fixes: #3497 PR-URL: #3500 Reviewed-By: Ben Noordhuis <[email protected]>
jasnell commented Oct 28, 2015
Landed in v4.x-staging bc2120c |
Fixes: #3497 PR-URL: #3500 Reviewed-By: Ben Noordhuis <[email protected]>
4 participants
Fixes: #3497
CI: https://ci.nodejs.org/job/node-test-pull-request/583/