<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - ECMAScript 2016: %TypedArray%.prototype.includes implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=159385#c19">Comment # 19</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - ECMAScript 2016: %TypedArray%.prototype.includes implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=159385">bug 159385</a>
from <span class="vcard"><a class="email" href="mailto:benjamin@webkit.org" title="Benjamin Poulain <benjamin@webkit.org>"> <span class="fn">Benjamin Poulain</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=282752&action=diff" name="attach_282752" title="Patch">attachment 282752</a> <a href="attachment.cgi?id=282752&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=282752&action=review">https://bugs.webkit.org/attachment.cgi?id=282752&action=review</a>
Second round of review:
<span class="quote">> Source/JavaScriptCore/ChangeLog:6
> + Reviewed by Benjamin Poulain.</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:211
> + TypedArrayType arrayType = exec->thisValue().getObject()->classInfo()->typedArrayStorageType;</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:216
> + if (!valueToFind.isNumber() && !valueToFind.isDouble())</span >
Isn't "isNumber()" a superset of "isDouble()"?
valueToFind.isNumber() false implies valueToFind.isDouble() false.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> + typename ViewClass::ElementType target = ViewClass::toAdaptorNativeFromValue(exec, valueToFind);</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:226
> + if (std::isnan(target)) {</span >
This can be eliminated at compile time with the test described above for ViewClass.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:236
> + if (JSValue::strictEqual(exec, currentValue, valueToFind))</span >
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.
<span class="quote">> LayoutTests/ChangeLog:13
> + * js/regress/script-tests/typed-array-includes.js: Added.</span >
Any reason this is in js/regress and not in js/
<span class="quote">> LayoutTests/js/regress/script-tests/typed-array-includes.js:1
> +function assert(a) {</span >
You can use shouldBeTrue(), available in js-test-pre.js.
<span class="quote">> LayoutTests/js/regress/script-tests/typed-array-includes.js:22
> + assert(!tArray.includes("abc"));</span >
It is worth testing all the types: undefined, null, empty string, regular object.
<span class="quote">> LayoutTests/js/regress/script-tests/typed-array-includes.js:96
> +</span >
Some things worth checking too:
Object.getPrototypeOf(TypedArray).prototype.name === "includes"
Object.getPrototypeOf(TypedArray).prototype.length === 1</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>