★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18744Nouvelle recherche | Portail wanayoo
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

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

@BridgeAR BridgeAR commented Feb 13, 2018

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 label Feb 13, 2018
starkwang
starkwang approved these changes Feb 13, 2018
View changes
Copy link
Contributor

@starkwang starkwang left a comment

LGTM

@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR commented Feb 13, 2018

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

@BridgeAR BridgeAR added the author ready label Feb 13, 2018
lpinca
lpinca reviewed Feb 13, 2018
View changes
lib/buffer.js Outdated
}
if (Number.isNaN(offset)) {
Copy link
Member

@lpinca lpinca Feb 13, 2018

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

@BridgeAR BridgeAR Feb 13, 2018

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 added 2 commits Feb 14, 2018
@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

@BridgeAR BridgeAR commented Feb 14, 2018

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 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

@richardlau richardlau Feb 14, 2018

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 changes
Copy link
Contributor

@mmarchini mmarchini left a comment

LGTM with @richardlau comment addressed

@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR commented Feb 16, 2018

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 issue 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 issue 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

@BridgeAR BridgeAR commented Feb 16, 2018

Landed in 43b8ce4, 96c57fb.

I fixed the typo accordingly.

@BridgeAR BridgeAR closed this Feb 16, 2018
MylesBorins pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau

@jasnell jasnell

@daynin daynin

@lpinca lpinca

@targos targos

@cjihrig cjihrig

@hiroppy hiroppy

@mmarchini mmarchini

@starkwang starkwang

Assignees
No one assigned
Labels
author ready lib / src
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet