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

[bugzilla-daemon at webkit.org]

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

Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #283116|review?                     |review-
              Flags|                            |

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

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

Third review round.
This is getting in really good shape. But I think I found a couple of bugs:

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:209
> +    if (!valueToFind.isNumber())
> +        return JSValue::encode(jsBoolean(false));

I believe having this here is a bug. It is not spec compliant.

If you check https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes, step 4. is calling ToInteger() on the index.
Here, if the argument zero is not a number, we exit *before* calling ToInteger.

Why is that important?
The second argument can be an object with "valueOf". This optimization will skip the call to valueOf.

You should add a test verifying that passing a string as first argument and an object as second argument calls valueOf() exactly once on the object.

> Source/JavaScriptCore/runtime/ToNativeFromValue.h:58
> +    return Adaptor::toNativeFromDouble(value.toNumber(exec), result);

I think this is a bug too.

You are calling toNumber() on the value.
If valueToFind is an Object, that will call valueOf() on it. If it's null or undefined, it will be converted to number too.

You just need to return false here.

You need a test covering this case.

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:104
> +        Type ans = static_cast<Type>(value);

Rename ans->integer?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:163
> +    static Type toNativeFromUint32(uint32_t value, Type& result)

Is this needed? Where is it called?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:177
> +        if (value < minValue || value > maxValue)
> +            return false;

This is not really enough because of double->float conversion.

A double can be bigger than minValue and less than maxValue yet not convert cleanly to a float. This happens if the double has a higher precision than the float.

What we need is (static_cast<double>(static_cast<Type>(value)) == value).
This ensure that the conversion to float and back to double does not lose precision.
This is valid because you have a NaN check above so we don't have to deal with non-finite numbers.

It's probably possible to write a test covering this.

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:278
> +    static bool toNativeFromUint32(uint32_t value, Type& result)

Is this needed?

> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:290
> +        int32_t ans = static_cast<int32_t>(value);

Rename ans->integer.
I believe you could even type it uint8_t and skip the "if (ans > maxValue || ans < minValue)" below.

-- 
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/20160709/a4b208a5/attachment-0001.html>