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

util: fix negative 0 check in inspect #17507

Closed
wants to merge 1 commit into from
Closed

util: fix negative 0 check in inspect #17507

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Dec 6, 2017

closes #17500

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 Dec 6, 2017
@apapirovski apapirovski requested a review from BridgeAR December 6, 2017 21:33
bnoordhuis
bnoordhuis reviewed Dec 6, 2017
View reviewed changes
lib/util.js Outdated
@@ -600,7 +600,7 @@ function formatNumber(fn, value) {
// Format -0 as '-0'. A `value === -0` check won't distinguish 0 from -0.
// Using a division check is currently faster than `Object.is(value, -0)`
// as of V8 6.1.
if (1 / value === -Infinity)
if (value === 0 && 1 / value === -Infinity)
Copy link
Member

Choose a reason for hiding this comment

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

if (Object.is(value, -0))

Copy link
Member

@apapirovski apapirovski Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The performance concern is real (3x slower for Object.is) but does it really matter in this case? It feels like this might be a micro-optimization too far.

Copy link
Member

Choose a reason for hiding this comment

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

It got better recently but those changes may not be in 6.3 yet. And yeah, I don't think Object.is() is going to be the expensive part here.

apapirovski
apapirovski approved these changes Dec 6, 2017
View reviewed changes
lib/util.js Outdated
@@ -600,7 +600,7 @@ function formatNumber(fn, value) {
// Format -0 as '-0'. A `value === -0` check won't distinguish 0 from -0.
// Using a division check is currently faster than `Object.is(value, -0)`
// as of V8 6.1.
Copy link
Member

Choose a reason for hiding this comment

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

The comment should be adjusted if this is being changed to Object.is again.

apapirovski
apapirovski reviewed Dec 6, 2017
View reviewed changes
lib/util.js Outdated
// Using a division check is currently faster than `Object.is(value, -0)`
// as of V8 6.1.
// `Object.is` is faster as of 6.3, and this code doesn't
// need to be very performant anyway
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was thinking something more along the lines of:

// Format -0 as '-0'. A `value === -0` check won't distinguish 0 from -0.

We don't really need to talk about the performance of it, IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

alright 😄

BridgeAR
BridgeAR requested changes Dec 7, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM but we definitely should add a test before landing.

@apapirovski
Copy link
Member

@BridgeAR would you mind reviewing this? It appears your feedback might've been addressed. Thank you!

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2017
@addaleax
Copy link
Member

addaleax commented Dec 8, 2017

CI: https://ci.nodejs.org/job/node-test-commit/14686/

@apapirovski
Copy link
Member

Landed in 86527bd

@apapirovski apapirovski closed this Dec 9, 2017
apapirovski pushed a commit that referenced this pull request Dec 9, 2017
@devsnek @apapirovski
util: fix negative 0 check in inspect
86527bd 
PR-URL: #17507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@devsnek devsnek deleted the patch-6 branch December 9, 2017 18:06
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@devsnek @MylesBorins
util: fix negative 0 check in inspect
74429e7 
PR-URL: #17507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@devsnek @MylesBorins
util: fix negative 0 check in inspect
8383c34 
PR-URL: #17507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@BridgeAR BridgeAR mentioned this pull request Dec 16, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@devsnek @gibfahn
util: fix negative 0 check in inspect
b718e45 
PR-URL: #17507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@devsnek @gibfahn
util: fix negative 0 check in inspect
3236944 
PR-URL: #17507
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@devsnek devsnek mentioned this pull request Dec 29, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@XadillaX XadillaX XadillaX approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

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.

Incorrect result when logging subnormal floating-point number to console.