[Webkit-unassigned] [Bug 148035] [ES6] Add TypedArray.prototype functionality.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=148035

--- Comment #19 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 259195
  --> https://bugs.webkit.org/attachment.cgi?id=259195
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259195&action=review

>> 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.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
>> +        mergeSort(this, length, comparator);
> 
> Stray newline.
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
>> +    for (var i = 0; i < kept.length; i++) {
> 
> No braces for single-line block.

Fixed.

>> 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

Added Comment

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
>> +        default: {
> 
> No braces for single line statement.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
>> +            }
> 
> It's good to break, even at the end.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
>> +    long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);
> 
> Why are they `long`?

I made them long so no overflow would occur in computations for count.

>> 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.

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.

>> 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.

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.

>> 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.

Yeah, you are probably right. I moved this to a builtin.

>> 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?

Fixed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150819/821d2e08/attachment-0001.html>