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: avoid use of arguments #11358

Closed
wants to merge 2 commits into from
Closed

buffer: avoid use of arguments #11358

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 13, 2017

Avoid use of arguments in Buffer.prototype.toString()

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

@jasnell
buffer: avoid use of arguments
97e336e 
Avoid use of arguments in Buffer.prototype.toString()
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 13, 2017
ChALkeR
ChALkeR approved these changes Feb 13, 2017
View reviewed changes
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

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

addaleax
addaleax approved these changes Feb 15, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion

lib/buffer.js Outdated
let result;
if (arguments.length === 0) {
result = this.utf8Slice(0, this.length);
} else {
result = slowToString.apply(this, arguments);
result = slowToString.call(this, encoding, start, end);
Copy link
Member

Choose a reason for hiding this comment

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

This is the only call site for slowToString, maybe we can drop the .call entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will require a bit of editing inside slowToString which assumes it is bound to this, but that's certainly doable.

@jasnell
buffer: refactor slowToString
f20117f 
Since slowToString only has one callsite, refactor to eliminate
the use of call.
@jasnell
Copy link
Member Author

jasnell commented Feb 15, 2017

@addaleax ... added a second commit that refactors slowToString(). PTAL

addaleax
addaleax approved these changes Feb 15, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM!

@jasnell
Copy link
Member Author

jasnell commented Feb 15, 2017

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

jasnell added a commit that referenced this pull request Feb 15, 2017
@jasnell
buffer: avoid use of arguments
1e21d05 
Avoid use of arguments in Buffer.prototype.toString()

PR-URL: #11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Feb 15, 2017
@jasnell
buffer: refactor slowToString
4b8b7e9 
Since slowToString only has one callsite, refactor to eliminate
the use of call.

PR-URL: #11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Feb 15, 2017

Landed in 1e21d05...4b8b7e9

@jasnell jasnell closed this Feb 15, 2017
@notarseniy notarseniy mentioned this pull request Feb 16, 2017
Closed
2 tasks
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
@jasnell @italoacasas
buffer: avoid use of arguments
f9c410a 
Avoid use of arguments in Buffer.prototype.toString()

PR-URL: nodejs#11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
@jasnell @italoacasas
buffer: refactor slowToString
5b8f289 
Since slowToString only has one callsite, refactor to eliminate
the use of call.

PR-URL: nodejs#11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@jasnell @italoacasas
buffer: avoid use of arguments
d08a8e6 
Avoid use of arguments in Buffer.prototype.toString()

PR-URL: #11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@jasnell @italoacasas
buffer: refactor slowToString
4ffad09 
Since slowToString only has one callsite, refactor to eliminate
the use of call.

PR-URL: #11358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

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.
7 participants
@jasnell @ChALkeR @addaleax @lpinca @cjihrig @joyeecheung @nodejs-github-bot