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.

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

util: add inspect suffix to BigInt64Array elements #21499

Conversation

not-an-aardvark
Copy link
Contributor

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

This commit updates util.inspect to add an n suffix to BigInts that
appear in BigInt64Arrays. BigInts are formatted with an n suffix in
most cases, but this did not occur in BigInt64Arrays due to an apparent
oversight where the implementation of inspect for typed arrays assumed
that all typed array elements are numbers.

@not-an-aardvark not-an-aardvark added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 24, 2018
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 24, 2018
This commit updates `util.inspect` to add an `n` suffix to BigInts that
appear in BigInt64Arrays. BigInts are formatted with an `n` suffix in
most cases, but this did not occur in BigInt64Arrays due to an apparent
oversight where the implementation of `inspect` for typed arrays assumed
that all typed array elements are numbers.
lib/util.js Outdated
@@ -897,8 +901,12 @@ function formatTypedArray(ctx, value, recurseTimes, keys) {
const maxLength = Math.min(Math.max(0, ctx.maxArrayLength), value.length);
const remaining = value.length - maxLength;
const output = new Array(maxLength + (remaining > 0 ? 1 : 0));
const elementFormatter =
types.isBigInt64Array(value) || types.isBigUint64Array(value) ?
Copy link
Member

Choose a reason for hiding this comment

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

typeof value[0] will be waaaaaay faster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would result in elementFormatter having the wrong value when the array is empty, although admittedly it wouldn't matter in that case because elementFormatter would never be called. I'll change it to use typeof.

lib/util.js Outdated
@@ -897,8 +901,11 @@ function formatTypedArray(ctx, value, recurseTimes, keys) {
const maxLength = Math.min(Math.max(0, ctx.maxArrayLength), value.length);
const remaining = value.length - maxLength;
const output = new Array(maxLength + (remaining > 0 ? 1 : 0));
const elementFormatter = typeof value[0] === 'number' ?
Copy link
Member

Choose a reason for hiding this comment

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

Please change it to:

const formatter = len !== 0 && typeof value[0] === 'number' ?
  formatNumber :
  formatBigInt;

Otherwise the OOB access for an empty TypedArray slows down all following calls.

@@ -897,8 +901,11 @@ function formatTypedArray(ctx, value, recurseTimes, keys) {
const maxLength = Math.min(Math.max(0, ctx.maxArrayLength), value.length);
const remaining = value.length - maxLength;
const output = new Array(maxLength + (remaining > 0 ? 1 : 0));
const elementFormatter = value.length > 0 && typeof value[0] === 'number' ?

This comment was marked as resolved.

@not-an-aardvark
Copy link
Contributor Author

@not-an-aardvark
Copy link
Contributor Author

Landed in 80496a5

@not-an-aardvark not-an-aardvark deleted the format-bigint64array-elements branch June 28, 2018 02:06
not-an-aardvark added a commit that referenced this pull request Jun 28, 2018
This commit updates `util.inspect` to add an `n` suffix to BigInts that
appear in BigInt64Arrays. BigInts are formatted with an `n` suffix in
most cases, but this did not occur in BigInt64Arrays due to an apparent
oversight where the implementation of `inspect` for typed arrays assumed
that all typed array elements are numbers.

PR-URL: #21499
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants