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.

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

buffer: simplify code #25151

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@BridgeAR
Copy link
Member

BridgeAR commented Dec 20, 2018

This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.

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: simplify code
This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.
@addaleax
Copy link
Member

addaleax left a comment

LGTM except for one question

Show resolved Hide resolved lib/buffer.js
@@ -637,21 +638,18 @@ Buffer.prototype.toString = function toString(encoding, start, end) {
}

const len = this.length;
if (len === 0)

This comment has been minimized.

@mscdex

mscdex Dec 20, 2018

Contributor

Why is this optimization removed?

This comment has been minimized.

@BridgeAR

BridgeAR Dec 20, 2018

Member

This will be detected below with the if (end <= start) check and it seemed an unnecessary check for the average case without hurting much in the empty buffer case.

@@ -637,21 +638,18 @@ Buffer.prototype.toString = function toString(encoding, start, end) {
}

This comment has been minimized.

@mscdex

mscdex Dec 20, 2018

Contributor

Minor nit: can this also be indented correctly?

This comment has been minimized.

@BridgeAR

BridgeAR Dec 23, 2018

Member

This seems correct? It's indented by exactly two spaces.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 23, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 24, 2018

Landed in 65d8179

@BridgeAR BridgeAR closed this Dec 24, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 24, 2018

buffer: simplify code
This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.

PR-URL: nodejs#25151
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 25, 2018

buffer: simplify code
This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.

PR-URL: #25151
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

MylesBorins added a commit that referenced this pull request Dec 26, 2018

buffer: simplify code
This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.

PR-URL: #25151
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

buffer: simplify code
This refactors some code for simplicity. It also removes a call
indirection used in the buffers custom inspect function.

PR-URL: nodejs#25151
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment