<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#c19">Comment # 19</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:keith_miller@apple.com" title="Keith Miller <keith_miller@apple.com>"> <span class="fn">Keith Miller</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>
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:26
>> +// Note that the intrisic @typedArrayLength checks the that the argument passed is a typed array.
>
> Does it throw if not? You should say so.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
>> + mergeSort(this, length, comparator);
>
> Stray newline.
>
> No braces for single-line block.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
>> + else {
>
> No braces for single-line block.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
>> + else {
>
> No braces for single-line block.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
>> + // FIXME: This should be a species constructor
>
> constructor => constructor.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
>> + // FIXME: This should be a species constructor
>
> constructor => constructor.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
>> + for (var i = 0; i < kept.length; i++) {
>
> No braces for single-line block.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
>> +void sortFloat(FloatType* floatArray, unsigned length)
>
> 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 >
Added Comment
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
>> +
>
> Stray newline.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
>> +
>
> Stray newline.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
>> + default: {
>
> No braces for single line statement.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
>> + }
>
> It's good to break, even at the end.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
>> + long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);
>
> Why are they `long`?</span >
I made them long so no overflow would occur in computations for count.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
>> + separator = { &comma, 1 };
>
> 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 >
I believe you are correct. I just copied this code from ArrayPrototype.cpp and didn't give it too much thought. We should probably fix this there as well.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
>> + separator = separatorString->view(exec);
>
> 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 >
I don't think there is an issue here. I believe JSString* lives in the js heap so it needs to be marked before it can be GCed.
<span class="quote">>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
>> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)
>
> No JavaScript action on this one? It would help with the function call.</span >
Yeah, you are probably right. I moved this to a builtin.
<span class="quote">>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
>> + return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));
>
> Could you add "Receiver should be a typed array view but was not an object" test cases for each function?</span >
Fixed.</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>