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

[bugzilla-daemon at webkit.org]

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

--- Comment #18 from Yusuke Suzuki <utatane.tea at gmail.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

Great, significant work :D

> 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

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:104
> +        unionArray[i].floatType = purifyNaN(unionArray[i].floatType);

I think, here, we can just write

for (unsigned i = 0; i < length; ++i)
   floatArray[i] = purifyNaN(floatArray[i]);

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:106
> +    IntegralType* intArray = bitwise_cast<IntegralType*>(unionArray);

Can we just write `bitwise_cast<IntegralType*>(floatArray)`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:112
> +    while (lastNegativeIndex < length && std::signbit(unionArray[++lastNegativeIndex].floatType)) { }

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

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

Why are they `long`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:182
> +    StringView separator;

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.

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

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

> Source/JavaScriptCore/runtime/JSObject.h:1492
> +    } while (false)

Nice!

> Source/JavaScriptCore/runtime/JSTypedArrayPrototypes.cpp:35
> +    CREATE_METHOD_TABLE(JSTypedArrayViewPrototype)};

We can just write `&Base::s_info` here.

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

-- 
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/e73199c1/attachment-0001.html>