Skip to content

util: use ES2015+ Object.is to check negative zero#11332

Closed
shinnn wants to merge 1 commit intonodejs:masterfrom
shinnn:object-is
Closed

util: use ES2015+ Object.is to check negative zero#11332
shinnn wants to merge 1 commit intonodejs:masterfrom
shinnn:object-is

Conversation

@shinnn
Copy link
Contributor

@shinnn shinnn commented Feb 13, 2017

The original code is written in 2013. Today we can use more simple way Object.is, which was introduced in ECMAScript 6, to check whether the value is negative zero or not.

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

util

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 13, 2017
mscdex
mscdex reviewed Feb 13, 2017
View reviewed changes
lib/util.js Outdated
Copy link
Contributor

@mscdex mscdex Feb 13, 2017

Choose a reason for hiding this comment

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

The removal of this line leaves the sentence from the previous line incomplete.

Copy link
Contributor Author

@shinnn shinnn Feb 13, 2017

Choose a reason for hiding this comment

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

Thanks. I missed the last comma (not a period) in the previous line.

@shinnn
util: use ES2015+ Object.is to check negative zero
4d9579c 
The original code nodejs@b3e4fc6 is written in 2013. Today we can use more simple way `Object.is`, which was introduced in ECMAScript 6, to check whether the value is negative zero or not.
bnoordhuis
bnoordhuis approved these changes Feb 13, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 13, 2017

@shinnn By the way, did you check that the benchmarks in benchmark/util didn't regress?

@shinnn
Copy link
Contributor Author

shinnn commented Feb 13, 2017 •
edited
Loading

did you check that the benchmarks in benchmark/util didn't regress?

On my machine,

# before
util/inspect.js n=5000000: 34,926.911787501784

# after
util/inspect.js n=5000000: 34,675.27026138018

However, I guess this change doesn't actually affect the existing util.inspect benchmark because the test fixture doesn't include any numbers.

node/benchmark/util/inspect.js

Line 13 in dcfda10

util.inspect({a: 'a', b: 'b', c: 'c', d: 'd'});

evanlucas
evanlucas approved these changes Feb 13, 2017
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM and test/parallel/test-util-inspect.js already has a test for this. Thanks!

jasnell pushed a commit that referenced this pull request Feb 16, 2017
@shinnn @jasnell
util: use ES2015+ Object.is to check negative zero
5ddf722 
Use `Object.is` to check whether the value is negative zero or not.

Ref: b3e4fc6
PR-URL: #11332
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 16, 2017

Landed in 5ddf722

@jasnell jasnell closed this Feb 16, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
@shinnn
util: use ES2015+ Object.is to check negative zero
271047a 
Use `Object.is` to check whether the value is negative zero or not.

Ref: nodejs@b3e4fc6
PR-URL: nodejs#11332
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@shinnn
util: use ES2015+ Object.is to check negative zero
3bdac54 
Use `Object.is` to check whether the value is negative zero or not.

Ref: b3e4fc6
PR-URL: #11332
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

This would need a backport PR to land in v4

jasnell pushed a commit that referenced this pull request Mar 7, 2017
@shinnn @jasnell
util: use ES2015+ Object.is to check negative zero
1ace4e2 
Use `Object.is` to check whether the value is negative zero or not.

Ref: b3e4fc6
PR-URL: #11332
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@shinnn @MylesBorins
util: use ES2015+ Object.is to check negative zero
e54b433 
Use `Object.is` to check whether the value is negative zero or not.

Ref: b3e4fc6
PR-URL: #11332
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+3 more reviewers

@mscdex mscdex mscdex left review comments

@evanlucas evanlucas evanlucas approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.