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 formatting of objects with SIMD enabled #7864

Closed
wants to merge 3 commits into from
Closed

util: fix formatting of objects with SIMD enabled #7864

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

When SIMD is enabled, util.format couldn’t display objects (with at least 1 key) because the formatter function got overridden.

@addaleax
util: fix formatting of objects with SIMD enabled
5e01208 
When SIMD is enabled, `util.format` couldn’t display objects
(with at least 1 key) because the formatter function got
overridden.
@addaleax addaleax added util Issues and PRs related to the built-in util module. dont-land-on-v4.x labels Jul 25, 2016
@bnoordhuis
Copy link
Member

For robustness and performance reasons (no double lookup), it might be better to set the formatter in the else block around line 510, but LGTM either way.

@addaleax
Copy link
Member Author

@bnoordhuis Updated, if I understood your suggestion correctly.

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

@bnoordhuis
Copy link
Member

Not quite what I had in mind but your approach is a bit better still. LGTM.

@bnoordhuis
Copy link
Member

New CI, last one failed with a Jenkins error on Windows: https://ci.nodejs.org/job/node-test-pull-request/3421/

@jasnell
Copy link
Member

jasnell commented Jul 29, 2016

oh, thank you for this, I'd noted this down a while back but never got around to it! This LGTM!

@targos
Copy link
Member

targos commented Jul 29, 2016

Shouldn't we run the test with --harmony-simd flag?

@addaleax
Copy link
Member Author

@targos I’ve added some tests from test-util-format.js to test-util-inspect-simd.js, too. Does that work for you? You’re making a good point but I think I’d rather have both code paths tested (SIMD on/off).

@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2016

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

@jasnell
Copy link
Member

jasnell commented Aug 1, 2016

Build bot failure on Windows in the last CI run, running again just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3487/

@targos
Copy link
Member

targos commented Aug 2, 2016

LGTM

@addaleax
Copy link
Member Author

addaleax commented Aug 2, 2016

That last CI failed completely, new attempt: https://ci.nodejs.org/job/node-test-commit/4370/

@addaleax
Copy link
Member Author

addaleax commented Aug 2, 2016

Landed in 1b24b37, thanks for the reviews!

@addaleax addaleax closed this Aug 2, 2016
@addaleax addaleax deleted the fix-util-format-simd branch August 2, 2016 13:08
addaleax added a commit that referenced this pull request Aug 2, 2016
@addaleax
util: fix formatting of objects with SIMD enabled
1b24b37 
When SIMD is enabled, `util.format` couldn’t display objects
(with at least 1 key) because the formatter function got
overridden.

PR-URL: #7864
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@addaleax @cjihrig
util: fix formatting of objects with SIMD enabled
0a07201 
When SIMD is enabled, `util.format` couldn’t display objects
(with at least 1 key) because the formatter function got
overridden.

PR-URL: #7864
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
@yosuke-furukawa yosuke-furukawa mentioned this pull request Sep 19, 2016
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
5 participants
@addaleax @bnoordhuis @jasnell @targos @cjihrig