More generic version of Array.concat builtin.

More generic version of Array.concat builtin.

Committed: http://code.google.com/p/v8/source/detail?r=4151

[antonm, 2010-03-16 04:20:52]

Lasse, Mads,

here comes promised more generic version of Array.concat.

Lasse, this patch should obsolete http://codereview.chromium.org/971001 if
submitted first

[Lasse Reichstein, 2010-03-16 09:50:14]

LGTM, and I'll ignore the other CL.

http://codereview.chromium.org/1036002/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/1036002/diff/1/2#newcode748
src/builtins.cc:748: result_len += len;
Might want comment stating why this can't overflow: (FixedArray::kMaxLength <
(max positive int / 2)), so being <= kMaxLength means that adding another
array's length won't overflow.

[Lasse Reichstein, 2010-03-16 10:33:30]

Bonus comment.

http://codereview.chromium.org/1036002/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/1036002/diff/1/2#newcode741
src/builtins.cc:741: FixedArray* elms = NULL;
elms isn't used anywhere, so the function with side-effect is wasted here.
Wouldn't it be simpler to just check manually:
 if(!arg->IsJSArray() || !JSArray::cast(arg)->HasFastElements()) {

[antonm, 2010-03-16 10:51:23]

Thanks a lot for review, Lasse.

Running throw the tests and submitting.

http://codereview.chromium.org/1036002/diff/1/2
File src/builtins.cc (right):

http://codereview.chromium.org/1036002/diff/1/2#newcode741
src/builtins.cc:741: FixedArray* elms = NULL;
On 2010/03/16 10:33:30, Lasse Reichstein wrote:
> elms isn't used anywhere, so the function with side-effect is wasted here.
> Wouldn't it be simpler to just check manually:
>  if(!arg->IsJSArray() || !JSArray::cast(arg)->HasFastElements()) {
> 

Sure.

http://codereview.chromium.org/1036002/diff/1/2#newcode748
src/builtins.cc:748: result_len += len;
On 2010/03/16 09:50:14, Lasse Reichstein wrote:
> Might want comment stating why this can't overflow: (FixedArray::kMaxLength <
> (max positive int / 2)), so being <= kMaxLength means that adding another
> array's length won't overflow.

Done.  And static assert added.