<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Add TypedArray.prototype functionality."
href="https://bugs.webkit.org/show_bug.cgi?id=148035#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Add TypedArray.prototype functionality."
href="https://bugs.webkit.org/show_bug.cgi?id=148035">bug 148035</a>
from <span class="vcard"><a class="email" href="mailto:utatane.tea@gmail.com" title="Yusuke Suzuki <utatane.tea@gmail.com>"> <span class="fn">Yusuke Suzuki</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=259195&action=diff" name="attach_259195" title="Patch">attachment 259195</a> <a href="attachment.cgi?id=259195&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=259195&action=review">https://bugs.webkit.org/attachment.cgi?id=259195&action=review</a>
Great, significant work :D
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
> +void sortFloat(FloatType* floatArray, unsigned length)</span >
I suggest you to write why this function works.
Or I suggest you to write this as a simple std::sort at first (and later, optimize it with the benchmark :D)
(the following one is my rough understanding),
IEEE 754 ensures that the mantissa and exponential part of the format (63bit) is ordered except for NaN.
Because IEEE 754 does not use complement representation for the negative values, the negative float numbers recognized as the integral numbers are reverse ordered.
We summarize the result of the comparison between the floating numbers recognized as the integral ones.
- <=> - = reversed (-30 > -20 = true)
+ <=> + = ordered (30 > 20 = true)
- <=> + = ordered (-30 > 20 = false)
+ <=> - = ordered (30 > -20 = true)
As a result, when sorting the array,
1. unsigned numbers are correctly ordered.
2. The order of unsigned and signed numbers are correctly kept.
3. But the order between signed numbers are reversed.
For NaN, we normalize the NaN to the specific representation; sign bit is 0, all exponential part is 1 and the MSB of the mantissa is 1.
So NaN is recognized as the largest integral numbers.
I suggest you to test with the following edge numbers,
1. positive infinity, negative infinity
2. -0.0, +0.0
3. NaN
4. denormalized numbers
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:104
> + unionArray[i].floatType = purifyNaN(unionArray[i].floatType);</span >
I think, here, we can just write
for (unsigned i = 0; i < length; ++i)
floatArray[i] = purifyNaN(floatArray[i]);
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:106
> + IntegralType* intArray = bitwise_cast<IntegralType*>(unionArray);</span >
Can we just write `bitwise_cast<IntegralType*>(floatArray)`?
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:112
> + while (lastNegativeIndex < length && std::signbit(unionArray[++lastNegativeIndex].floatType)) { }</span >
Let's replace `{ }` to `;`.
I think we can check it by using intArray (intArray[i] < 0) instead of using `std::signbit`.
And this code has the issue of overrun. When `length == 1`, this will access `unionArray[1]`.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
> + long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);</span >
Why are they `long`?
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:182
> + StringView separator;</span >
StringView is just the view and it does not have any ownership of the held string value.
So when using it, we need to take care of the lifetime of the owner string.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
> + separator = { &comma, 1 };</span >
Is this &comma addressing safe? This comma address is the address of the stack.
When this brace is ended, the lifetime of the variable `comma` is finished.
I think we need to change it to the following.
const LChar* comma = ",";
separator = { comma, 1 };
String Literal is allocated in the static area and its lifetime is the same to the program.
<span class="quote">> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
> + separator = separatorString->view(exec);</span >
If we would like to keep separator valid, we need to maintain separatorString alive.
I suggest you to move `JSString* separatorString` to the function top-level block to make it explicit that separatorString is alive during this function.
<span class="quote">> Source/JavaScriptCore/runtime/JSObject.h:1492
> + } while (false)</span >
Nice!
<span class="quote">> Source/JavaScriptCore/runtime/JSTypedArrayPrototypes.cpp:35
> + CREATE_METHOD_TABLE(JSTypedArrayViewPrototype)};</span >
We can just write `&Base::s_info` here.
<span class="quote">> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
> + return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));</span >
Could you add "Receiver should be a typed array view but was not an object" test cases for each function?</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>