[Webkit-unassigned] [Bug 159385] ECMAScript 2016: %TypedArray%.prototype.includes implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159385

--- Comment #19 from Benjamin Poulain <benjamin at webkit.org> ---
Comment on attachment 282752
  --> https://bugs.webkit.org/attachment.cgi?id=282752
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282752&action=review

Second round of review:

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by Benjamin Poulain.

You don't need to modify this field.

The tool that lands the page (webkit-patch) replaces "NOBODY (OOPS!)" by the name of the reviewer found on bugzilla.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211
> +    TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType;

This is less efficient than it should.

You are getting the array type at runtime.
This condition can be fully determined at compile time from the template argument ViewClass.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216
> +    if (!valueToFind.isNumber() && !valueToFind.isDouble())

Isn't "isNumber()" a superset of "isDouble()"?

valueToFind.isNumber() false implies valueToFind.isDouble() false.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> +    typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind);

I don't think that works for includes.

For example, Uint8ClampedAdaptor will clamp positive values to 255, that's not what we need here.
IntegralTypedArrayAdaptor does ToInt32 on the input, that's not what we need either.

Your test should have extensive coverage of those crazy cases.

----

What you probably need is a new Adaptor function that returns a value in the right type or an error.

For example, if you pass a double to an integer type array, it returns the integer if it fits in the array type or fail.
E.g.:
    -The double 256.0 works for int16, fails for int8
    -The double 254.1 fails for all integers.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:226
> +    if (std::isnan(target)) {

This can be eliminated at compile time with the test described above for ViewClass.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:236
> +            if (JSValue::strictEqual(exec, currentValue, valueToFind))

I think it is not a good idea to use strictEqual() for performance.
The type of the Array is known, and the type of the valueToFind can be converted to that type.

> LayoutTests/ChangeLog:13
> +        * js/regress/script-tests/typed-array-includes.js: Added.

Any reason this is in js/regress and not in js/

> LayoutTests/js/regress/script-tests/typed-array-includes.js:1
> +function assert(a) {

You can use shouldBeTrue(), available in js-test-pre.js.

> LayoutTests/js/regress/script-tests/typed-array-includes.js:22
> +    assert(!tArray.includes("abc"));

It is worth testing all the types: undefined, null, empty string, regular object.

> LayoutTests/js/regress/script-tests/typed-array-includes.js:96
> +

Some things worth checking too:
Object.getPrototypeOf(TypedArray).prototype.name === "includes"
Object.getPrototypeOf(TypedArray).prototype.length === 1

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160705/f3acdb2b/attachment-0001.html>