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.

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: remove unreachable code #12438

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 16, 2017

46f202 introduced changes that cause a previously reachable error
condition to be unreachable. Remove the code.

The error condition checks that byteLength() doesn't return -1. But
that will only happen if byteLength() receives an encoding that is
falsy. However the call to byteLength() is in an else block that is
only reachable if encoding is a string with a length greater than 0.
So byteLength() will never return -1 in this situation.

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

buffer

46f202 introduced changes that cause a previously reachable error
condition to be unreachable. Remove the code.

The error condition checks that `byteLength()` doesn't return -1. But
that will only happen if `byteLength()` receives an encoding that is
falsy. However the call to `byteLength()` is in an `else` block that is
only reachable if `encoding` is a string with a length greater than 0.
So `byteLength()` will never return `-1` in this situation.
@Trott Trott added the buffer Issues and PRs related to the buffer subsystem. label Apr 16, 2017
@benjamingr
Copy link
Member

Can I see a performance benchmark?

Unreachable code might sometimes have surprising effects on the performance :)

@mscdex
Copy link
Contributor

mscdex commented Apr 16, 2017

I think #12439 should be considered instead, which fixes the unintentional backwards incompatibility introduced in #12361, making this particular piece of code reachable again.

@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

I also prefer #12439 instead.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

#12439 has landed. Closing this. Can reopen if necessary.

@jasnell jasnell closed this Apr 18, 2017
@Trott Trott deleted the rm-dead-code branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants