Introduce Array.splice builtin.

Introduce Array.splice builtin.

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

[antonm, 2010-02-17 07:10:22]

Mads,

the last of Array methods for you.

Some notes.

1) I found that our current implementation apparently is not complaint to
ECMAScript standard (if I am reading it correctly), is it well-known fact or do
I misinterpret something?  I even found a bug in the spec which sounds partially
suspicious :)  And I would especially appreciate comments on wording in
comments.

2) I am not sure that couple git cl and codereview correctly process
moves---array-functions-prototype-2.js is former array-splice.js which actually
tests various methods.  I needed that name to keep our tests symmetric.

I ran the batch of changes through layout tests and they went fine.  Trying to
run through Sputnik.  If there are some other tests, I'd be happy to know.

[Lasse Reichstein, 2010-02-17 09:11:45]

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

http://codereview.chromium.org/618002/diff/1/3#newcode533
src/builtins.cc:533: // Not conformant, but that's the way it is currently.
This is also what Safari does.

http://codereview.chromium.org/618002/diff/1/3#newcode548
src/builtins.cc:548: // but current implementation behaves differently.
This is also Safari compatible.
(Arguably it's also consistent with slice and with String's substring when
omitting the second parameter).

[antonm, 2010-02-17 09:29:32]

Thanks a lot, Lasse.  I too checked if Safari behaves this way.  Mildly curious
who cares about 15.4.4.12 though :)  Jokes aside, is there an important
ECMAScript implementation which implements it right?  Any chances that standard
should be amended?  Or all the implementations are different enough to never
find the common ground?

yours,
anton.

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

http://codereview.chromium.org/618002/diff/1/3#newcode548
src/builtins.cc:548: // but current implementation behaves differently.
On 2010/02/17 09:11:46, Lasse Reichstein wrote:
> This is also Safari compatible.
> (Arguably it's also consistent with slice and with String's substring when
> omitting the second parameter).

I'm somewhat surprised that undefined here is treated differently than argument
omission---undefined behaves like 0 (which is expected), but single arg form
behaves like len (which is slice compatible as you say).

[Mads Ager (chromium), 2010-02-17 09:44:22]

LGTM with comments.

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

http://codereview.chromium.org/618002/diff/1/3#newcode533
src/builtins.cc:533: // Not conformant, but that's the way it is currently.
Could you use the comment from array.js which is more informative than this one?

  // SpiderMonkey and JSC return undefined in the case where no
  // arguments are given instead of using the implicit undefined
  // arguments.  This does not follow ECMA-262, but we do the same for
  // compatibility.

http://codereview.chromium.org/618002/diff/1/3#newcode534
src/builtins.cc:534: if (n_arguments == 0)
Braces around the body or single-line.

http://codereview.chromium.org/618002/diff/1/3#newcode548
src/builtins.cc:548: // but current implementation behaves differently.
Again, please use the more informative comment from array.js for this one.

http://codereview.chromium.org/618002/diff/1/5
File test/mjsunit/array-functions-prototype-2.js (right):

http://codereview.chromium.org/618002/diff/1/5#newcode34
test/mjsunit/array-functions-prototype-2.js:34: var LARGE = 40000000;
Please make sure that this test does not take a long time.  This should be a
simple test that can be run very quickly.  Benchmarks are fine but should not be
part of the mjsunit test suite.

http://codereview.chromium.org/618002/diff/1/5#newcode37
test/mjsunit/array-functions-prototype-2.js:37: // Nicer for firefox 1.5. 
Unless you uncomment the following two lines,
Let's remove this.  This should test correctness and not performance.

http://codereview.chromium.org/618002/diff/1/5#newcode145
test/mjsunit/array-functions-prototype-2.js:145: // IE, Safari get this right.
We should just test what we believe is the right thing here.  Let's remove the
comments about what the other engines do.  These comments are bound to become
wrong when the other implementations change and all that matters is that we test
the correct behavior.

http://codereview.chromium.org/618002/diff/1/5#newcode200
test/mjsunit/array-functions-prototype-2.js:200: // IE, Safari get this wrong.
Ditto.

[antonm, 2010-02-17 10:33:10]

Thanks a lot, Lasse and Mads.

Submitting with proper svn move.

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

http://codereview.chromium.org/618002/diff/1/3#newcode533
src/builtins.cc:533: // Not conformant, but that's the way it is currently.
On 2010/02/17 09:44:23, Mads Ager wrote:
> Could you use the comment from array.js which is more informative than this
one?
> 
>   // SpiderMonkey and JSC return undefined in the case where no
>   // arguments are given instead of using the implicit undefined
>   // arguments.  This does not follow ECMA-262, but we do the same for
>   // compatibility.
> 

Done and updated in regard of TraceMonkey which started to return empty array. 
KJS renamed to JSC as per IM discussion.

http://codereview.chromium.org/618002/diff/1/3#newcode534
src/builtins.cc:534: if (n_arguments == 0)
On 2010/02/17 09:44:23, Mads Ager wrote:
> Braces around the body or single-line.

Done.

http://codereview.chromium.org/618002/diff/1/3#newcode548
src/builtins.cc:548: // but current implementation behaves differently.
On 2010/02/17 09:44:23, Mads Ager wrote:
> Again, please use the more informative comment from array.js for this one.

Done.

http://codereview.chromium.org/618002/diff/1/5
File test/mjsunit/array-functions-prototype-2.js (right):

http://codereview.chromium.org/618002/diff/1/5#newcode34
test/mjsunit/array-functions-prototype-2.js:34: var LARGE = 40000000;
On 2010/02/17 09:44:23, Mads Ager wrote:
> Please make sure that this test does not take a long time.  This should be a
> simple test that can be run very quickly.  Benchmarks are fine but should not
be
> part of the mjsunit test suite.

As discussed by IM, I just attempted to rename this file, didn't modify it.