[webkit-reviews] review denied: [Bug 131609] Array.prototype.concat should allocate output storage only once. : [Attachment 229273] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied  review:
Bug 131609: Array.prototype.concat should allocate output storage only once.
https://bugs.webkit.org/show_bug.cgi?id=131609

Attachment 229273: Patch
https://bugs.webkit.org/attachment.cgi?id=229273&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229273&action=review


>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:423
>> +	for (size_t i = 0; i <= argCount; ++i) {
> 
> Yet this is size_t. What guarantees we don’t overflow the unsigned?

Why <= argCount? That means we always walk off the end of the argument list. 
What am i missing?

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:428
>> +	    curArg = exec->uncheckedArgument(i);
> 
> A little lame to do this one extra time at the end of the loop but then
discard the result.

ah i see. do {} while loop instead maybe?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:442
>	       for (unsigned k = 0; k < length; ++k) {
> -		   JSValue v = getProperty(exec, curObject, k);
> +		   JSValue v = getProperty(exec, currentArray, k);

currentArray is an object, i would do a canGetFastIndex, and fastGet -- so two
loops but the get should be much faster.