These patches add the following global constructors:
- ArrayBuffer([length])
- %TypedArray%([length])
- %TypedArray%([buffer, [start, [end]]])
- %TypedArray%([array])
- %TypedArray%([typedarray])
For %TypedArray% the constructors are: Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array, Float32Array, Float64Array and Uint8ClampedArray, BigUint64Array and BigInt64Array.
The following properties were implemented:
- ArrayBuffer.isView(obj)
- ArrayBuffer[Symbol.species]
- ArrayBuffer.prototype.constructor
- ArrayBuffer.prototype.byteLength
- ArrayBuffer.prototype.slice([start, [end]])
- ArrayBuffer.prototype[Symbol.toStringTag]
- ArrayBuffer,.prototype.toString()
- %TypedArray%.prototype.constructor
- %TypedArray%.prototype.set(array [, start])
- %TypedArray%.prototype.slice([start, [end]])
- %TypedArray%.prototype.toString()
- %TypedArray%.prototype.buffer
- %TypedArray%.prototype.byteLength
- %TypedArray%.prototype.byteOffset
- %TypedArray%.prototype.length
Patches: https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10
Test262:
=== Summary ===
- Ran 32306 tests
- - Passed 10688 tests (33.1%)
- - Failed 21618 tests (66.9%)
+ - Passed 11085 tests (34.3%)
+ - Failed 21221 tests (65.7%)
@tiago4orion
Hi!
Thank you for the patches!
Looks unneeded:
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L246
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L294
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L497
and below
maybe '&args[1] -> value' ?
Style: new line before else, please.
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L1000
njs_is_null_or_undefined -> njs_is_undefined
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L529
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L540
return NJS_ERROR
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L107
There's no need to set size to 0 here, as it's already initialized to 0 before.
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L901
It's better to add a comment here describing why the return value of njs_value_to_number() isn't checked.
Thanks, I'll update it soon with improvements.
For the function TypedArray.prototype.toString, I was lazy and just copied the typedarray content into a newly allocated array and used its own toString method... but I'll improve by properly implementing the TypedArray.prototype.join and just point the toString to Array.prototype.toString builtin function as the spec says. The current implementation also does not free the temporary array allocated in it.
I updated the patches.
I implemented TypedArray.prototype.join and pointed TypedArray.prototype.toString to njs_array_prototype_to_string, as says the spec.
I also implemented the TypedArray.prototype.copyWithin function.
@tiago4orion
An actual [proto]type hierarchy defined by the spec is Uint8Array -> TypedArray -> Object :
$ node --use-strict
> var u8 = new Uint8Array([1,2,3]);
undefined
> var u8p = Object.getPrototypeOf(u8);
undefined
> Object.getOwnPropertyDescriptors(u8p);
{ constructor:
{ value: [Function: Uint8Array],
writable: true,
enumerable: false,
configurable: true },
BYTES_PER_ELEMENT:
{ value: 1,
writable: false,
enumerable: false,
configurable: false } }
> var u8pp = Object.getPrototypeOf(u8p);
undefined
> new u8pp.constructor([1,2,3])
Thrown:
TypeError: Abstract class TypedArray not directly constructable
But there is no TypedArray in your implementation.
Is it an intentional optimization?
No. I was planning to contribute this in a future patch.
We are very close to having that now, but I'm not really sure how to add that behavior. I think it will require to create an own prototype creator function (njs_typed_array_prototype_create) that exposes the internal TypedArray constructor, right?
No. I was planning to contribute this in a future patch.
Yes, I think we can leave this as is for now.
Patches updated: Moved the isnan change to arraybuffer patch.
@tiago4orion
Please, consider and apply the following improvements over the first patch
https://gist.github.com/2308c3f96dafc091e3f18e62f90c044d
@tiago4orion
Consider the following changes:
https://gist.github.com/xeioex/b77a5c977dc2fd2b34d383f423ea14a1
1) Introduced ToIndex() conversion primitive from the spec. (will commit to upstream soon)
2) Introduced the ArrayBuffer object. (your patch)
3) fix1.patch
4) Introduced TypedArray objects. (your patch)
5) fix2_2.patch (so far I simplified only njs_typed_array_constructor()).
Tomorrow I will continue review/improvement cycle for the second patch.
@tiago4orion Updated https://gist.github.com/xeioex/9bea539dd687b1d3ccaa2ca9c40b863e
1) Introduced ToIndex() conversion primitive from the spec.
2) Improved object type initialization structure. (to make generic constructors, so your patches can be simplified even more).
3) Introduced the ArrayBuffer object. (your patch)
4) fix1.patch
5) Introduced TypedArray objects. (your patch)
6) fix2_2.patch
I removed BigUint64Array, BigInt64Array because they require BigInt primitive which is a separate task. After it, adding them would be a trivial patch.
Will continue tomorrow.
@tiago4orion
BTW (just letting you know),
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L2454
this code is a bad idea
1) it slows down other code passes
2) the code should be moved into njs_property_query()
because many other internal functions also need this
3) they also need a descriptor property like with NJS_ARRAY see this https://github.com/nginx/njs/blob/master/src/njs_value.c#L709
For this to work as expected
> var arr = new Int32Array(10); Object.getOwnPropertyDescriptor(arr, '3')
{ value: 0,
writable: true,
enumerable: true,
configurable: false }
On it.
@tiago4orion
Changed in the committed patch:
1) Introduced proper types hierarchy (TypedArray->%TypedArray%->Object).
2) added %TypedArray%.prototype.fill().
3) %TypedArray%.prototype.join() rewritten from scratch according to spec (using njs_chb_t API).
4) TypedArray constructors rewritten according to spec.
5) njs.dump() now outputs TypedArray instances as ${TypedArray.name} [...].
6) Many other small changes (Style, test262 related).
Thank you for contributing.
- - Passed 10974 tests (33.9%)
- - Failed 21397 tests (66.1%)
+ - Passed 11421 tests (35.3%)
+ - Failed 20950 tests (64.7%)
Most helpful comment
Yes, I think we can leave this as is for now.