[webkit-reviews] review canceled: [Bug 234604] Implement Change Array by copy proposal : [Attachment 457159] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <ashvayka at apple.com> has canceled Aditi Singh
<asingh at igalia.com>'s request for review:
Bug 234604: Implement Change Array by copy proposal
https://bugs.webkit.org/show_bug.cgi?id=234604

Attachment 457159: Patch

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




--- Comment #20 from Alexey Shvayka <ashvayka at apple.com> ---
Comment on attachment 457159
  --> https://bugs.webkit.org/attachment.cgi?id=457159
Patch

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

> Source/JavaScriptCore/ChangeLog:7
> +	   This patch implements proposal Change Array By Copy. The proposal
provides additional 

nit: please add an extra newline before this.

> Source/JavaScriptCore/ChangeLog:10
> +	   Since, a call to intrinsic functions:
Array.prototype.sort(comparefn) and 

It would be nice if you could expand the ChangeLog regarding why we treat
TypedArray's sort() and Array's sort() differently (i.e. not all sites use
TypedArray), and why it's important to avoid

    function sort(cmp) { return @arraySort. at call(this, cmp); }

for performance reasons.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1353
> +	   JSValue arraySort =
jsCast<JSFunction*>(arrayPrototype()->getDirect(vm,
vm.propertyNames->builtinNames().sortPublicName()));

nit: JSFunction* arraySort = ...

> JSTests/stress/unscopables.js:1
> +//@ requireOptions("--useAtMethod=1 --useChangeArrayByCopyMethods=1")

Aha, JSC tests fail because expected syntax is:

    //@ requireOptions("--useAtMethod=1", "--useChangeArrayByCopyMethods=1")

> JSTests/stress/unscopables.js:14
> +    test(String(Object.keys(unscopables).sort()),
"at,copyWithin,entries,fill,find,findIndex,findLast,findLastIndex,flat,flatMap,
includes,keys,toReversed,toSorted,toSpliced,values,with");

Should "with" be removed from here?