<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#c6">Comment # 6</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:ticaiolima@gmail.com" title="Caio Lima <ticaiolima@gmail.com>"> <span class="fn">Caio Lima</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=159385#c4">comment #4</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=282639&action=diff" name="attach_282639" title="RFC Patch">attachment 282639</a> <a href="attachment.cgi?id=282639&action=edit" title="RFC Patch">[details]</a></span>
> RFC Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=282639&action=review">https://bugs.webkit.org/attachment.cgi?id=282639&action=review</a>
>
> 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
> > + <a href="https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes">https://tc39.github.io/ecma262/2016/#sec-%typedarray%.prototype.includes</a>
>
> Be careful to use spaces for indentation.
> We avoid mixing tabs and spaces, the rule is to always use spaces.</span >
Sorry, my vim is not configured correctly. Thanks for point it =).
<span class="quote">> > 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).</span >
I added coverage, but I didn't consider cases about property descriptor. I am going to update it soon.
<span class="quote">> > 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.</span >
I agree with you. I double checked the spec and confirm it. However, I am not sure if it is possible create an Array with length > INT_MAX. I need to check it.
<span class="quote">> > 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
> }</span >
Nice! However I decided pick the C++ version in the new Patch.
<span class="quote">> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:222
> > + if (array[index] == target)
>
> Isn't that missing the NaN case?</span >
Now I am considering it. However I have a doubt with suspicious cases:
1. If the searchElement is an Object or a String? In IntXArray, they are converted to 0, so "new UInt8Array([0, 1, 2]).includes("abc"); // is true". It is not just the case of %TypedArray%.prototype.includes, but also %TypedArray%.prototype.indexOf. I didn't find any information about it in the Spec. I checked v8 implementation and they return false to these cases. IMHO, I think it is the best result, since "new UInt8Array([0, 1, 2]).includes("abc");" returning "true" is a potential unpredictable bug source.
<span class="quote">> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:223
> > + return JSValue::encode(JSValue(JSValue::JSTrue));
>
> You can also use jsBoolean(true/false) to produce a boolean JSValue.</span >
Thank you for this Tip!</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>