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

[bugzilla-daemon at webkit.org]

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

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #259195|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #16 from Geoffrey Garen <ggaren 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

Looks good but needs some improvement.

> LayoutTests/ChangeLog:8
> +        Added tests for the TypedArray.prototype functions

functions => functions.

> LayoutTests/ChangeLog:9
> +        all the tests use the typedarray-test-helper-function.js

all => All

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

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:119
> +    function merge(dst, src, srcIndex, srcEnd, width, comparator)

Because typed arrays do not require stable sort (since their entries are all primitives), it would be nice to use a faster sorting algorithm, like quick sort. You should switch to quick sort in a follow-up patch.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
> +
> +        mergeSort(this, length, comparator);

Stray newline.

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
> +    else {

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
> +    else {

No braces for single-line block.

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

constructor => constructor.

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

constructor => constructor.

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

No braces for single-line block.

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

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:102
> +    TypeConverter* unionArray = bitwise_cast<TypeConverter*>(floatArray);

This.... probably doesn't do what you think it does.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:110
> +    // When we sort floating point numbers as ints comparing two negative numbers will
> +    // give the wrong ordering so we need to reverse these numbers.

Can you pass a comparator to std::sort that will do this naturally?

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

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
> +        default: {

No braces for single line statement.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
> +            }

It's good to break, even at the end.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
> +template<typename ViewClass>
> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)

No JavaScript action on this one? It would help with the function call.

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