<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #283116 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<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#c38">Comment # 38</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=283116&action=diff" name="attach_283116" title="Patch">attachment 283116</a> <a href="attachment.cgi?id=283116&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=283116&action=review">https://bugs.webkit.org/attachment.cgi?id=283116&action=review</a>
Third review round.
This is getting in really good shape. But I think I found a couple of bugs:
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:209
> + if (!valueToFind.isNumber())
> + return JSValue::encode(jsBoolean(false));</span >
I believe having this here is a bug. It is not spec compliant.
If you check <a href="https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes">https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes</a>, 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.
<span class="quote">> Source/JavaScriptCore/runtime/ToNativeFromValue.h:58
> + return Adaptor::toNativeFromDouble(value.toNumber(exec), result);</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:104
> + Type ans = static_cast<Type>(value);</span >
Rename ans->integer?
<span class="quote">> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:163
> + static Type toNativeFromUint32(uint32_t value, Type& result)</span >
Is this needed? Where is it called?
<span class="quote">> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:177
> + if (value < minValue || value > maxValue)
> + return false;</span >
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.
<span class="quote">> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:278
> + static bool toNativeFromUint32(uint32_t value, Type& result)</span >
Is this needed?
<span class="quote">> Source/JavaScriptCore/runtime/TypedArrayAdaptors.h:290
> + int32_t ans = static_cast<int32_t>(value);</span >
Rename ans->integer.
I believe you could even type it uint8_t and skip the "if (ans > maxValue || ans < minValue)" below.</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>