-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Refactored typed arrays #18559
Refactored typed arrays #18559
Conversation
e7ae252
to
fc79d85
Compare
@NaridaL sorry for the delay, can you refresh the PR? |
Changes look good to me. @rbuckton any comments? |
While we're at it, does it make sense to rename TypedArray to BaseTypedArray have a separate |
previous: ``` PS C:\Users\aval\tsdev\TypeScript> node built\local\tsc.js --diagnostics --p src\compiler\tsconfig.json Files: 36 Lines: 93186 Nodes: 445312 Identifiers: 168285 Symbols: 71668 Types: 32088 Memory used: 371617K I/O read: 0.02s I/O write: 0.17s Parse time: 0.71s Bind time: 0.62s Check time: 2.90s Emit time: 2.06s Total time: 6.29s ``` now: ``` PS C:\Users\aval\tsdev\TypeScript> node built\local\tsc.js --diagnostics --p src\compiler\tsconfig.json Files: 36 Lines: 91099 Nodes: 439748 Identifiers: 166769 Symbols: 71022 Types: 31979 Memory used: 369763K I/O read: 0.02s I/O write: 0.03s Parse time: 0.69s Bind time: 0.59s Check time: 2.92s Emit time: 1.89s Total time: 6.09s ``` Fixes microsoft#15402
fc79d85
to
82f75bb
Compare
I rebased it. |
* number of bytes could not be allocated an exception is raised. | ||
*/ | ||
interface Int8Array { | ||
interface TypedArray<T> { |
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.
What value does the generic T
give you here? You could just use the this
type wherever you use T
below without making TypedArray
generic.
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.
See #18425
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.
and slice and subarray don't necessarily return this if I'm not mistaken
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 think we have since fixed some of the underlying issues that made it slower.
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.
and slice and subarray don't necessarily return this if I'm not mistaken
that i do not know how to address with this
...
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.
Yes, the thisArg params can be typed with this
. Is there a disadvantage to using T for for the slice/subarray return types?
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.
We can't properly constrain the generic, so you could write a TypedArray<HTMLElement>
which would not make sense.
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.
As nice as it would be to completely deduplicate TypedArray
, I don't believe that is a completely viable option at this point. As we cannot properly constrain T
, I would recommend we do not make TypedArray
generic at this time.
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.
Having TypedArray generic allows for the following construct, which would type all the methods which use SpeciesConstructor correctly.
class MyInt8Array extends (Int8Array as TypedArray<MyInt8Array>) {
[Symbol.species]: MyInt8Array
}
If the goal is to have a common unambiguous (non-generic) supertype to which only true TypedArrays can be assigned, I would suggest #18559 (comment) which, along with an appropriate brand field, would more effectively prevent someone from assigning something which looks like a TypedArray to something which expects a real one.
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.
What's the issue with interface TypedArray<T extends Uint16Array | Uint8Array | ...> {
if you want to constrain TypedArray?
@@ -215,7 +215,7 @@ interface String { | |||
[Symbol.iterator](): IterableIterator<string>; | |||
} | |||
|
|||
interface Int8Array { | |||
interface TypedArray<T> { |
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 should not be generic.
@@ -231,243 +231,14 @@ interface Int8Array { | |||
values(): IterableIterator<number>; | |||
} | |||
|
|||
interface Int8ArrayConstructor { | |||
new (elements: Iterable<number>): Int8Array; | |||
interface TypedArrayConstructor<T> { |
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 would rather we leave out TypedArrayConstructor
entirely and leave the individual constructor interfaces.
} | ||
|
||
interface Float64Array { | ||
interface TypedArray<T> { |
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 should not be generic.
@@ -1570,7 +1566,7 @@ interface Int8Array { | |||
* @param thisArg An object to which the this keyword can refer in the callbackfn function. | |||
* If thisArg is omitted, undefined is used as the this value. | |||
*/ | |||
filter(callbackfn: (value: number, index: number, array: Int8Array) => any, thisArg?: any): Int8Array; | |||
filter(callbackfn: (value: number, index: number, array: this) => any, thisArg?: any): T; |
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.
Rather than return T
, either return this
or return TypedArray
with a subtyped overload in each specific typed-array constructor (e.g. return Int32Array
in the Int32Array
interface).
@@ -1639,7 +1635,7 @@ interface Int8Array { | |||
* @param thisArg An object to which the this keyword can refer in the callbackfn function. | |||
* If thisArg is omitted, undefined is used as the this value. | |||
*/ | |||
map(callbackfn: (value: number, index: number, array: Int8Array) => number, thisArg?: any): Int8Array; | |||
map(callbackfn: (value: number, index: number, array: this) => number, thisArg?: any): T; |
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.
Rather than return T
, either return this
or return TypedArray
with a subtyped overload in each specific typed-array constructor (e.g. return Int32Array
in the Int32Array
interface).
@@ -1708,7 +1704,7 @@ interface Int8Array { | |||
* @param start The beginning of the specified portion of the array. | |||
* @param end The end of the specified portion of the array. | |||
*/ | |||
slice(start?: number, end?: number): Int8Array; | |||
slice(start?: number, end?: number): T; |
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.
Rather than return T
, either return this
or return TypedArray
with a subtyped overload in each specific typed-array constructor (e.g. return Int32Array
in the Int32Array
interface).
|
||
/** | ||
* Reverses the elements in an Array. | ||
*/ | ||
reverse(): Int8Array; | ||
reverse(): T; |
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.
Rather than return T
, either return this
or return TypedArray
with a subtyped overload in each specific typed-array constructor (e.g. return Int32Array
in the Int32Array
interface).
* number of bytes could not be allocated an exception is raised. | ||
*/ | ||
interface Int8Array { | ||
interface TypedArray<T> { |
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.
As nice as it would be to completely deduplicate TypedArray
, I don't believe that is a completely viable option at this point. As we cannot properly constrain T
, I would recommend we do not make TypedArray
generic at this time.
* at begin, inclusive, up to end, exclusive. | ||
* @param begin The index of the beginning of the array. | ||
* @param end The index of the end of the array. | ||
*/ | ||
subarray(begin: number, end?: number): Int8Array; | ||
subarray(begin: number, end?: number): T; |
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.
Rather than return T
, either return this
or return TypedArray
with a subtyped overload in each specific typed-array constructor (e.g. return Int32Array
in the Int32Array
interface).
new(length: number): Int8Array; | ||
new(arrayOrArrayBuffer: ArrayLike<number> | ArrayBufferLike): Int8Array; | ||
new(buffer: ArrayBufferLike, byteOffset: number, length?: number): Int8Array; | ||
interface TypedArrayConstructor<T> { |
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.
For now, we should leave the existing typed array constructors as is rather than introduce TypedArrayConstructor
.
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.
Alternatively, we could try the following definition:
interface TypedArrayConstructor {
prototype: TypedArray;
new (length: number): this["prototype"];
new (arrayOrArrayBuffer: ArrayLike<number> | ArrayBufferLike): this["prototype"];
new (buffer: ArrayBufferLike, byteOffset: number, length?: number): this["prototype"];
of(...items: number[]): this["prototype"];
from(arrayLike: ArrayLike<number>, mapfn?: (v: number, k: number) => number, thisArg?: any): this["prototype"];
}
interface Int8ArrayConstructor extends TypedArrayConstructor {
prototype: Int8Array;
}
Though its possible this could cause problems with Symbol.species
.
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
Fixes #15402
I based this on #18555, otherwise I'm fairly certain it would lead to merge conflicts...
Let me know if this is a problem and I'll rebase it.