[webkit-reviews] review granted: [Bug 202582] Update Array.prototype.sort to be consistent with tightened spec : [Attachment 408753] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 202582: Update Array.prototype.sort to be consistent with tightened spec
https://bugs.webkit.org/show_bug.cgi?id=202582

Attachment 408753: Patch

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




--- Comment #12 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 408753
  --> https://bugs.webkit.org/attachment.cgi?id=408753
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:33
> +	   Optimization #1: replace char-by-char comparison loop with >
operator, aligning
> +	   JSC with V8 and SpiderMonkey. This semantically equivalent change
alone is a ~15%
> +	   progression for string sort.

Lol, nice.

> Source/JavaScriptCore/ChangeLog:46
> +	   Optimization #4: always return sorted array instead of copying, even
if it's the buffer.
> +	   Also, create the buffer with correct length.

Nice.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:326
> +	   return aString > bString ? 1 : -1;

Nice.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:358
> +	       @appendMemcpy(receiver, sorted, 0);

Let's check the receiver is JSArray since @appendMemcpy only accepts JSArrays.
So, receiver and sorted must be JSArray. While sorted is always JSArray,
receiver is not (user can call Array#sort with non arrays).
@isJSArray(receiver) is required.
And I think `@assert(@isJSArray(sorted))` here would be nice.
Can you add a test exercising this case?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:383
> +		       right++;

Use `++right` for consistency since we are not using post-increment feature
here.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:391
> +		       right++;

Ditto.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:397
> +	       left++;

Ditto.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:426
> +		   dst++;

Ditto.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:437
> +		   dst++;

Ditto.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:446
> +		   @putByValDirect(buckets, c, [entry]);

Let's use `[ entry ]` (adding spaces before and after [ ]).