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

buffer: do not always use defaults by BridgeAR · Pull Request #20054 · nodejs/node · GitHub

/ node Public

buffer: do not always use defaults - #20054

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:stricter-buffer-defaults
ConversationCommits (1)ChecksFiles changed
Closed

buffer: do not always use defaults#20054
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:stricter-buffer-defaults

Conversation

BridgeAR commented Apr 15, 2018

Copy link
Copy Markdown
Member

The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

I checked this against gzemnid and it did not show a single module using
this pattern. I personally say it is a bug if the user calls these functions
without arguments, because it is not clear what happens.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

buffer: do not always use defaults
d986951 
The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.
BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 15, 2018
BridgeAR requested review from ChALkeR and addaleax April 15, 2018 20:12
nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Apr 15, 2018
BridgeAR requested a review from a team April 15, 2018 20:12

BridgeAR commented Apr 15, 2018

Copy link
Copy Markdown
Member Author

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

addaleax approved these changes Apr 15, 2018
View reviewed changes

addaleax left a comment

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

LGTM

BridgeAR requested a review from a team April 15, 2018 21:38
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 15, 2018

rvagg commented Apr 16, 2018

Copy link
Copy Markdown
Member

LGTM I suppose, seems a bit late to be squeezing something like this into 10 though

mcollina approved these changes Apr 16, 2018
View reviewed changes

mcollina left a comment

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

LGTM

ChALkeR approved these changes Apr 18, 2018
View reviewed changes

ChALkeR left a comment •
edited
Loading

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

LGTM.

Nit though: part of test style changes look unrelated, and I am not sure if those should be done in a semver-major commit. /cc @MylesBorins

BridgeAR commented Apr 25, 2018

Copy link
Copy Markdown
Member Author

@ChALkeR I can split the tests into a different PR if you want. But since it is work: are you fine with landing it as is?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 29, 2018
buffer: do not always use defaults
60b5b38 
The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

PR-URL: nodejs#20054
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

BridgeAR commented Apr 29, 2018

Copy link
Copy Markdown
Member Author

No one complaint in the last 11 days, so I went ahead and landed it as is.

Landed in 60b5b38

BridgeAR closed this Apr 29, 2018
targos added a commit to targos/node that referenced this pull request Jun 6, 2018
test: move test-readuint to test-buffer-readuint
831ac4c 
Backport of a changed test from
nodejs#20054
targos mentioned this pull request Jun 6, 2018
Closed
targos added a commit that referenced this pull request Jun 8, 2018
test: move test-readuint to test-buffer-readuint
fd7964f 
Backport of a changed test from
#20054

PR-URL: #21170
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos added a commit that referenced this pull request Jun 13, 2018
test: move test-readuint to test-buffer-readuint
2ff4704 
Backport of a changed test from
#20054

PR-URL: #21170
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR deleted the stricter-buffer-defaults branch April 1, 2019 23:40
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

mcollina mcollina approved these changes
ChALkeR ChALkeR approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
trivikr trivikr approved these changes
+1 more reviewer
XadillaX XadillaX approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL