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.

Sign up for GitHub

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

Jump to bottom

buffer: fix value check for writeUInt{B,L}E #3500

Closed
wants to merge 1 commit into from
Closed

buffer: fix value check for writeUInt{B,L}E #3500

wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

bnoordhuis
bnoordhuis reviewed Oct 23, 2015
View reviewed changes
lib/buffer.js
if (!noAssert)
checkInt(this, value, offset, byteLength, Math.pow(2, 8 * byteLength), 0);
checkInt(this, value, offset, byteLength, maxBytes, 0);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 23, 2015
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

@trevnorris ... would you want this in v4.x?

@trevnorris
Copy link
Contributor Author

@jasnell bug fix, so I think so.

@trevnorris
Copy link
Contributor Author

@bnoordhuis Addressed comments.

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

@trevnorris
Copy link
Contributor Author

Two unrelated failures. Everything else looks good.

@trevnorris trevnorris force-pushed the buf-fix-uint-write branch 2 times, most recently from 540da8d to 67d4e96 Compare October 26, 2015 20:10
@bnoordhuis
Copy link
Member

LGTM. Consider adding tests for byteLength > 1.

@trevnorris
Copy link
Contributor Author

@bnoordhuis do these (https://github.com/nodejs/node/pull/3500/files#diff-aa771a8dc1afdde910c49517a74a0ddeR143) not cover that?

@bnoordhuis
Copy link
Member

I only see tests for byteLength == 0 and 1 or am I missing something?

trevnorris added a commit that referenced this pull request Oct 26, 2015
@trevnorris
buffer: fix value check for writeUInt{B,L}E
3308e5e 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris
Copy link
Contributor Author

Thanks for the review. Landed in 3308e5e.

@trevnorris trevnorris closed this Oct 26, 2015
@trevnorris trevnorris deleted the buf-fix-uint-write branch October 26, 2015 20:59
trevnorris added a commit that referenced this pull request Oct 26, 2015
@trevnorris @rvagg
buffer: fix value check for writeUInt{B,L}E
6a04cc0 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Oct 27, 2015
Merged
trevnorris added a commit that referenced this pull request Oct 28, 2015
@trevnorris @jasnell
buffer: fix value check for writeUInt{B,L}E
bc2120c 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

Landed in v4.x-staging bc2120c

@rvagg rvagg mentioned this pull request Oct 28, 2015
Closed
trevnorris added a commit that referenced this pull request Oct 29, 2015
@trevnorris @jasnell
buffer: fix value check for writeUInt{B,L}E
1d0f2cb 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell mentioned this pull request Oct 29, 2015
Closed
This was referenced Apr 29, 2023
Open
Open
This was referenced Jun 22, 2023
Open
Open
Open
Open
Open
@leonardoadame leonardoadame mentioned this pull request Jul 9, 2023
Open
@aliscco aliscco mentioned this pull request Oct 27, 2023
Open
@aliscco aliscco mentioned this pull request Nov 28, 2023
Open
This was referenced Nov 29, 2023
Open
Open
@aliscco aliscco mentioned this pull request Dec 2, 2023
Open
@leonardoadame leonardoadame mentioned this pull request Dec 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trevnorris @jasnell @bnoordhuis @mscdex