util: fix negative 0 check in inspect by devsnek · Pull Request #17507 · nodejs/node (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

closes #17500

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

util

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

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

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.

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.

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

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.

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

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.

Choose a reason for hiding this comment

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

alright 😄

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.

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

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

apapirovski pushed a commit that referenced this pull request

Dec 9, 2017

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

MylesBorins pushed a commit that referenced this pull request

Dec 12, 2017

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

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