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

lib: switch to Number.isNaN #18744

Closed
wants to merge 2 commits into from
Closed

lib: switch to Number.isNaN #18744

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

This enables the eslint no-self-compare rule.

The reason is to switch to the more natural Number.isNaN()check. I benchmarked both versions and they are on par so we do not have to use those old ones anymore.

I also removed a NaN check that was obsolete and added a test for that.

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
Affected core subsystem(s)

lib

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 13, 2018
starkwang
starkwang approved these changes Feb 13, 2018
View reviewed changes
Copy link
Contributor

@starkwang starkwang left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member Author

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2018
lpinca
lpinca reviewed Feb 13, 2018
View reviewed changes
lib/buffer.js Outdated
}
if (Number.isNaN(offset)) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not merging this with the offset === 0 case on line 1000?

Copy link
Member Author

Choose a reason for hiding this comment

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

NaN is just a safety check. The other cases are not. Therefore I prioritized the other case. Due to your comment I actually saw that it is possible to move the NaN check down to be the last.

@BridgeAR
buffer: remove obsolete NaN check
7d42edc 
These two NaN entries are not necessary and we can safely remove them.
@BridgeAR
lib: switch to Number.isNaN
ae2fd64 
Number.isNaN is now as fast as `val !== val`. Switch to the more
readable version. Also switch all `isNaN` to `Number.isNaN`.
@BridgeAR
Copy link
Member Author

I realized that another NaN check was obsolete, so I removed it as well and moved a NaN check further down. I also changed all isNaN to Number.isNaN. This is fine since we always had numbers in those cases.

PTAL

richardlau
richardlau reviewed Feb 14, 2018
View reviewed changes
test/parallel/test-buffer-arraybuffer.js
@@ -148,3 +148,6 @@ b.writeDoubleBE(11.11, 0, true);
message: '"length" is outside of buffer bounds'
});
}

// Test a array like entry with the length set to NaN.
Copy link
Member

Choose a reason for hiding this comment

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

an array-like?

mmarchini
mmarchini approved these changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@mmarchini mmarchini 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 @richardlau comment addressed

@BridgeAR
Copy link
Member Author

New CI before landing https://ci.nodejs.org/job/node-test-pull-request/13205/

(I am going to fix the typo when landing).

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
@BridgeAR
buffer: remove obsolete NaN check
43b8ce4 
These two NaN entries are not necessary and we can safely remove them.

PR-URL: nodejs#18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
@BridgeAR
lib: switch to Number.isNaN
96c57fb 
Number.isNaN is now as fast as `val !== val`. Switch to the more
readable version. Also switch all `isNaN` to `Number.isNaN`.

PR-URL: nodejs#18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 43b8ce4, 96c57fb.

I fixed the typo accordingly.

@BridgeAR BridgeAR closed this Feb 16, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
buffer: remove obsolete NaN check
4073ab1 
These two NaN entries are not necessary and we can safely remove them.

PR-URL: #18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
lib: switch to Number.isNaN
6604a2d 
Number.isNaN is now as fast as `val !== val`. Switch to the more
readable version. Also switch all `isNaN` to `Number.isNaN`.

PR-URL: #18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
buffer: remove obsolete NaN check
f3ab106 
These two NaN entries are not necessary and we can safely remove them.

PR-URL: #18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
lib: switch to Number.isNaN
16aeddd 
Number.isNaN is now as fast as `val !== val`. Switch to the more
readable version. Also switch all `isNaN` to `Number.isNaN`.

PR-URL: #18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
buffer: remove obsolete NaN check
10d4f58 
These two NaN entries are not necessary and we can safely remove them.

PR-URL: nodejs#18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
lib: switch to Number.isNaN
505d412 
Number.isNaN is now as fast as `val !== val`. Switch to the more
readable version. Also switch all `isNaN` to `Number.isNaN`.

PR-URL: nodejs#18744
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR deleted the switch-nan branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@starkwang starkwang starkwang approved these changes

@mmarchini mmarchini mmarchini approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@daynin daynin daynin approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.