buffer: fix value check for writeUInt{B,L}E #3500
Conversation
| if (!noAssert) | ||
| checkInt(this, value, offset, byteLength, Math.pow(2, 8 * byteLength), 0); | ||
| checkInt(this, value, offset, byteLength, maxBytes, 0); |
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.
|
@trevnorris ... would you want this in v4.x? |
|
@jasnell bug fix, so I think so. |
trevnorris
force-pushed
the
buf-fix-uint-write
branch
from
3d2fd99
to
425d681
Compare
|
@bnoordhuis Addressed comments. New CI: https://ci.nodejs.org/job/node-test-pull-request/620/ |
|
Two unrelated failures. Everything else looks good. |
trevnorris
force-pushed
the
buf-fix-uint-write
branch
2 times, most recently
from
540da8d
to
67d4e96
Compare
|
LGTM. Consider adding tests for byteLength > 1. |
|
@bnoordhuis do these (https://github.com/nodejs/node/pull/3500/files#diff-aa771a8dc1afdde910c49517a74a0ddeR143) not cover that? |
|
I only see tests for byteLength == 0 and 1 or am I missing something? |
trevnorris
force-pushed
the
buf-fix-uint-write
branch
from
67d4e96
to
559e66b
Compare
|
Thanks for the review. Landed in 3308e5e. |
|
Landed in v4.x-staging bc2120c |
Fixes: #3497
CI: https://ci.nodejs.org/job/node-test-pull-request/583/