[webkit-reviews] review granted: [Bug 182440] [JSC] Implement Array.prototype.flatMap and Array.prototype.flatten : [Attachment 333164] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 182440: [JSC] Implement Array.prototype.flatMap and Array.prototype.flatten
https://bugs.webkit.org/show_bug.cgi?id=182440

Attachment 333164: Patch

https://bugs.webkit.org/attachment.cgi?id=333164&action=review




--- Comment #34 from Darin Adler <darin at apple.com> ---
Comment on attachment 333164
  --> https://bugs.webkit.org/attachment.cgi?id=333164
Patch

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

> Source/JavaScriptCore/builtins/ArrayPrototype.js:743
> +function arraySpeciesCreate(array, length)

Do we have test coverage for all the special cases in here?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:756
> +    // We have this check so that if some array from a different global
object
> +    // calls this map they don't get an array with the Array.prototype of
the
> +    // other global object.
> +    if (arrayConstructorInRealm !== constructor &&
@isArrayConstructor(constructor))
> +	   return @newArrayWithSize(length);

Test for this case?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:781
> +		       @throwTypeError("flatten array exceeds 2**53 - 1");

This error string looks different in style from some others.

Is there some way to test this case, or would the test run too slowly?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:800
> +    var depthNum = 1;
> +    var depth = @argument(0);
> +    if (depth !== @undefined)
> +	   depthNum = @toInteger(depth);

I would have expected a ? : for this sort of thing:

    var depthInteger = depth === @undefined ? 1 : @toInteger(depth);

Assuming we generate good code for such expressions. Also makes me wonder if we
should make a variety of @toInteger where you can pass a default value in
rather than separately doing a check.