[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 #4 from Benjamin Poulain <benjamin at webkit.org> ---
Comment on attachment 282639
  --> https://bugs.webkit.org/attachment.cgi?id=282639
RFC Patch

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

An implementation of TypedArray's includes, that's awesome!

Some comments bellow:

> Source/JavaScriptCore/ChangeLog:10
> +				%TypedArray%.prototype.includes following spec 22.2.3.14
> +				https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes

Be careful to use spaces for indentation.
We avoid mixing tabs and spaces, the rule is to always use spaces.

> Source/JavaScriptCore/ChangeLog:19
> +

Don't forget to add test coverage too!

You should have tests in LayoutTests/js that try various use cases and verify the correctness.
There should also be a test getting the property descriptor and checking its properties (name, enumerable, etc).

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:355
> +        fromIndex = arguments[1] | 0;

This looks suspicious to me.

The specs says:
    Let n be ? ToInteger(fromIndex). (If fromIndex is undefined, this step produces the value 0.)

ToInteger() produces integer for values > INT_MAX.

x |Â 0 is a ToInt32() on x. The bounds are [INT_MIN, INT_MAX]

Looking at Array.prototype, I suspect we have a bug there.

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:370
> +        if (searchElement === currentElement || (searchElement !== searchElement && currentElement !== currentElement))

The loop would benefit from being specialized on searchElement !== searchElement.

if (searchElement === searchElement) {
    ... Loop searching for searchElement
} else {
    ... Loop searching for NaN
}

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> +        if (array[index] == target)

Isn't that missing the NaN case?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223
> +            return JSValue::encode(JSValue(JSValue::JSTrue));

You can also use jsBoolean(true/false) to produce a boolean JSValue.

-- 
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/20160704/45aa2c75/attachment.html>