Skip to content

util: improve empty typed array inspection - #22284

Closed
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:fast-empty-typedarray
Closed

util: improve empty typed array inspection#22284
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:fast-empty-typedarray

Conversation

@BridgeAR

Copy link
Copy Markdown
Member

They should be aligned with all other empty objects. Therefore the
whitespace is removed and they got a fast path 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

They should be aligned with all other empty objects. Therefore the
whitespace is removed and they got a fast path for that.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 12, 2018
@jasnell

jasnell commented Aug 12, 2018

Copy link
Copy Markdown
Member

I really don't want this to be semver-major but the output change may require it. Thoughts @nodejs/tsc?

@devsnekdevsnek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm without semver-major. the docs explicitly say not to use the output of util.inspect programmatically.

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think in this case it’s okay to land it as a non-breaking change if CITGM is okay with it.

(I’d generally keep it a case-by-case decision, though.)

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 13, 2018
@BridgeAR

Copy link
Copy Markdown
MemberAuthor

I just pushed another commit to prevent this path from being taken in case showHidden is set to true.

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

dnalborczyk pushed a commit to dnalborczyk/node that referenced this pull request Aug 15, 2018
They should be aligned with all other empty objects. Therefore the
whitespace is removed and they got a fast path for that.
PR-URL: nodejs#22284
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

CITGM runs came out fine.

@BridgeAR

Copy link
Copy Markdown
MemberAuthor

Landed in db6a246

@targostargos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 19, 2018
@targos

Copy link
Copy Markdown
Member

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

@targos

Copy link
Copy Markdown
Member

Landed easily after #21869

targos pushed a commit that referenced this pull request Aug 19, 2018
They should be aligned with all other empty objects. Therefore the
whitespace is removed and they got a fast path for that.
PR-URL: #22284
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
They should be aligned with all other empty objects. Therefore the
whitespace is removed and they got a fast path for that.
PR-URL: #22284
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@targostargos mentioned this pull request Sep 5, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@BridgeAR@nodejs-github-bot@jasnell@targos@addaleax@lpinca@JungMinu@devsnek@trivikr