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

events: remove unnecessary checks #9330

Merged
merged 1 commit into from
Oct 31, 2016
Merged

events: remove unnecessary checks #9330

merged 1 commit into from
Oct 31, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 27, 2016

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

events

Description of change

This commit removes two truthy checks for object properties that are immediately followed by a strict equality check. The other item in the comparison is guaranteed to be a function by this point in the code, so the truthy check is redundant.

@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 27, 2016
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. Out of curiosity, are you locating these bits of unreachable code by looking at the test coverage?

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 28, 2016

Not for this particular PR, but for the other similar PRs that I opened recently, yes.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 28, 2016

@Trott
Copy link
Member

Trott commented Oct 28, 2016

I'm stopping the freebsd11-x64 build on CI for this because it looks like it detached from Jenkins. Until it stops, I don't think the host won't be free for other jobs to use. :-(

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

This commit removes two truthy checks for object properties that
are immediately followed by a strict equality check. The other
item in the comparison is guaranteed to be a function by this
point in the code, so the truthy check is redundant.

PR-URL: nodejs#9330
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig merged commit 7f909c3 into nodejs:master Oct 31, 2016
@cjihrig cjihrig deleted the events branch October 31, 2016 19:58
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
This commit removes two truthy checks for object properties that
are immediately followed by a strict equality check. The other
item in the comparison is guaranteed to be a function by this
point in the code, so the truthy check is redundant.

PR-URL: #9330
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

@cjihrig is this applicable to v4 or v6?

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 22, 2016

If the code is on the other branches, then I'd say it's applicable. It's just a JS simplification.

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This commit removes two truthy checks for object properties that
are immediately followed by a strict equality check. The other
item in the comparison is guaranteed to be a function by this
point in the code, so the truthy check is redundant.

PR-URL: #9330
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This commit removes two truthy checks for object properties that
are immediately followed by a strict equality check. The other
item in the comparison is guaranteed to be a function by this
point in the code, so the truthy check is redundant.

PR-URL: #9330
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants