-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
util: add types.isFloat16Array()
#57879
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// Flags: --experimental-vm-modules --expose-internals --allow-natives-syntax | ||
// Flags: --experimental-vm-modules --expose-internals --allow-natives-syntax --js-float16array | ||
// TODO(LiviaMedeiros): once `Float16Array` is unflagged in v8, remove `--js-float16array` above | ||
'use strict'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think we shouldn't land this until this becomes stable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think doing it the other way around is actually good so that the new method is available as soon as it becomes stable. It will likely not be used before that anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is already stable, it is stage 4 – and if you meant the V8 implementation, that's really not necessary IMO, precisely because we can trust it will follow the spec. |
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
|
@@ -9,6 +10,9 @@ const { JSStream } = internalBinding('js_stream'); | |
|
||
const external = (new JSStream())._externalStream; | ||
|
||
// TODO(LiviaMedeiros): once linter recognizes `Float16Array`, remove next line | ||
const { Float16Array } = globalThis; | ||
|
||
for (const [ value, _method ] of [ | ||
[ external, 'isExternal' ], | ||
[ new Date() ], | ||
|
@@ -38,6 +42,7 @@ for (const [ value, _method ] of [ | |
[ new Int8Array() ], | ||
[ new Int16Array() ], | ||
[ new Int32Array() ], | ||
[ new Float16Array() ], | ||
[ new Float32Array() ], | ||
[ new Float64Array() ], | ||
[ new BigInt64Array() ], | ||
|
@@ -102,6 +107,9 @@ for (const [ value, _method ] of [ | |
assert(!types.isInt32Array({ [Symbol.toStringTag]: 'Int32Array' })); | ||
assert(types.isInt32Array(vm.runInNewContext('new Int32Array'))); | ||
|
||
assert(!types.isFloat16Array({ [Symbol.toStringTag]: 'Float16Array' })); | ||
assert(types.isFloat16Array(vm.runInNewContext('new Float16Array'))); | ||
|
||
assert(!types.isFloat32Array({ [Symbol.toStringTag]: 'Float32Array' })); | ||
assert(types.isFloat32Array(vm.runInNewContext('new Float32Array'))); | ||
|
||
|
@@ -127,6 +135,7 @@ for (const [ value, _method ] of [ | |
const int8Array = new Int8Array(arrayBuffer); | ||
const int16Array = new Int16Array(arrayBuffer); | ||
const int32Array = new Int32Array(arrayBuffer); | ||
const float16Array = new Float16Array(arrayBuffer); | ||
const float32Array = new Float32Array(arrayBuffer); | ||
const float64Array = new Float64Array(arrayBuffer); | ||
const bigInt64Array = new BigInt64Array(arrayBuffer); | ||
|
@@ -141,6 +150,7 @@ for (const [ value, _method ] of [ | |
const fakeInt8Array = { __proto__: Int8Array.prototype }; | ||
const fakeInt16Array = { __proto__: Int16Array.prototype }; | ||
const fakeInt32Array = { __proto__: Int32Array.prototype }; | ||
const fakeFloat16Array = { __proto__: Float16Array.prototype }; | ||
const fakeFloat32Array = { __proto__: Float32Array.prototype }; | ||
const fakeFloat64Array = { __proto__: Float64Array.prototype }; | ||
const fakeBigInt64Array = { __proto__: BigInt64Array.prototype }; | ||
|
@@ -164,6 +174,10 @@ for (const [ value, _method ] of [ | |
Object.setPrototypeOf(new Int16Array(arrayBuffer), Int16Array.prototype); | ||
const stealthyInt32Array = | ||
Object.setPrototypeOf(new Int32Array(arrayBuffer), Int32Array.prototype); | ||
const stealthyFloat16Array = | ||
Object.setPrototypeOf( | ||
new Float16Array(arrayBuffer), Float16Array.prototype | ||
); | ||
const stealthyFloat32Array = | ||
Object.setPrototypeOf( | ||
new Float32Array(arrayBuffer), Float32Array.prototype | ||
|
@@ -191,6 +205,7 @@ for (const [ value, _method ] of [ | |
int8Array, fakeInt8Array, stealthyInt8Array, | ||
int16Array, fakeInt16Array, stealthyInt16Array, | ||
int32Array, fakeInt32Array, stealthyInt32Array, | ||
float16Array, fakeFloat16Array, stealthyFloat16Array, | ||
float32Array, fakeFloat32Array, stealthyFloat32Array, | ||
float64Array, fakeFloat64Array, stealthyFloat64Array, | ||
bigInt64Array, fakeBigInt64Array, stealthyBigInt64Array, | ||
|
@@ -208,6 +223,7 @@ for (const [ value, _method ] of [ | |
int8Array, stealthyInt8Array, | ||
int16Array, stealthyInt16Array, | ||
int32Array, stealthyInt32Array, | ||
float16Array, stealthyFloat16Array, | ||
float32Array, stealthyFloat32Array, | ||
float64Array, stealthyFloat64Array, | ||
bigInt64Array, stealthyBigInt64Array, | ||
|
@@ -222,6 +238,7 @@ for (const [ value, _method ] of [ | |
int8Array, stealthyInt8Array, | ||
int16Array, stealthyInt16Array, | ||
int32Array, stealthyInt32Array, | ||
float16Array, stealthyFloat16Array, | ||
float32Array, stealthyFloat32Array, | ||
float64Array, stealthyFloat64Array, | ||
bigInt64Array, stealthyBigInt64Array, | ||
|
@@ -248,6 +265,9 @@ for (const [ value, _method ] of [ | |
isInt32Array: [ | ||
int32Array, stealthyInt32Array, | ||
], | ||
isFloat16Array: [ | ||
float16Array, stealthyFloat16Array, | ||
], | ||
isFloat32Array: [ | ||
float32Array, stealthyFloat32Array, | ||
], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done fully on user land , why are we exposing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be inconsistent to not expose the 16 bit version next to all others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure consistency should be enough to expand the API surface. Is there any other reasoning for adding this other than consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the scope of this PR, the aim is on having this particular function on
node:internal/util/types
. It is re-exported asnode:util/types
as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be done in userland easily, though, because you'd have to extract the Symbol.toStringTag getter. Consistency is a pretty strong justification for extending any API surface, on its own merits.