FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: fix value check for writeUInt{B,L}E by trevnorris · Pull Request #3500 · nodejs/node · GitHub

/ node Public

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

Closed
trevnorris wants to merge 1 commit into
nodejs:masterfrom
trevnorris:buf-fix-uint-write
ConversationCommits (1)ChecksFiles changed
Closed

buffer: fix value check for writeUInt{B,L}E#3500
trevnorris wants to merge 1 commit into
nodejs:masterfrom
trevnorris:buf-fix-uint-write

Conversation

trevnorris commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

bnoordhuis reviewed Oct 23, 2015
View reviewed changes
Comment thread lib/buffer.js Outdated

bnoordhuis Oct 23, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

trevnorris Oct 23, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the buffer Issues and PRs related to the buffer subsystem. label Oct 23, 2015

jasnell commented Oct 26, 2015

Copy link
Copy Markdown
Member

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

trevnorris commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

@jasnell bug fix, so I think so.

trevnorris commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Addressed comments.

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

trevnorris commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

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 October 26, 2015 20:10

bnoordhuis commented Oct 26, 2015

Copy link
Copy Markdown
Member

LGTM. Consider adding tests for byteLength > 1.

trevnorris commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

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

bnoordhuis commented Oct 26, 2015

Copy link
Copy Markdown
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
buffer: fix value check for writeUInt{B,L}E
3308e5e 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

trevnorris commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Landed in 3308e5e.

trevnorris closed this Oct 26, 2015
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
buffer: fix value check for writeUInt{B,L}E
6a04cc0 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg mentioned this pull request Oct 27, 2015
Merged
trevnorris added a commit that referenced this pull request Oct 28, 2015
buffer: fix value check for writeUInt{B,L}E
bc2120c 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging bc2120c

rvagg mentioned this pull request Oct 28, 2015
Closed
trevnorris added a commit that referenced this pull request Oct 29, 2015
buffer: fix value check for writeUInt{B,L}E
1d0f2cb 
Fixes: #3497
PR-URL: #3500
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 mentioned this pull request Jul 9, 2023
Open
aliscco mentioned this pull request Oct 27, 2023
Open
aliscco mentioned this pull request Nov 28, 2023
Open
This was referenced Nov 29, 2023
Open
Open
aliscco mentioned this pull request Dec 2, 2023
Open
leonardoadame mentioned this pull request Dec 3, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL