Introduce builtin for Array.shift function.

Introduce builtin for Array.shift function.

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

[antonm, 2010-02-12 14:07:52]

Mads,

sorry, I messed up with git cl and it opened a new issue for me.

[Mads Ager (chromium), 2010-02-15 07:47:02]

LGTM

Are the cases with no elements on the object itself and elements present/absent
in the prototype already covered by tests?  If not, please add some tests to
your array-shift test to cover those cases as well.

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

http://codereview.chromium.org/606017/diff/1/3#newcode354
src/builtins.cc:354: // Set the length.
Move the setting on the new length after the optional getting on the first
element from the prototype (which is part of getting the first element so
belongs with elms->get(0) above).

http://codereview.chromium.org/606017/diff/1/3#newcode361
src/builtins.cc:361: for (int i = 0; i < len - 1; i++) {
Add a comment for consistency with the rest of the function?  "Shift the
elements."

http://codereview.chromium.org/606017/diff/1/5
File test/mjsunit/array-shift.js (right):

http://codereview.chromium.org/606017/diff/1/5#newcode28
test/mjsunit/array-shift.js:28: // Check that shifting array of holes keeps it
as arrary of holes
arrary -> array

[antonm, 2010-02-15 09:15:22]

Thanks, Mads.

I've added a test into array-shift.js which is pretty much the test from
array-functions-prototype.js.  If it's not enough, just let me know and I'd add
some more tests.

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

http://codereview.chromium.org/606017/diff/1/3#newcode354
src/builtins.cc:354: // Set the length.
On 2010/02/15 07:47:03, Mads Ager wrote:
> Move the setting on the new length after the optional getting on the first
> element from the prototype (which is part of getting the first element so
> belongs with elms->get(0) above).

Done.

http://codereview.chromium.org/606017/diff/1/3#newcode361
src/builtins.cc:361: for (int i = 0; i < len - 1; i++) {
On 2010/02/15 07:47:03, Mads Ager wrote:
> Add a comment for consistency with the rest of the function?  "Shift the
> elements."

Done.

http://codereview.chromium.org/606017/diff/1/5
File test/mjsunit/array-shift.js (right):

http://codereview.chromium.org/606017/diff/1/5#newcode28
test/mjsunit/array-shift.js:28: // Check that shifting array of holes keeps it
as arrary of holes
On 2010/02/15 07:47:03, Mads Ager wrote:
> arrary -> array

Done.

[Mads Ager (chromium), 2010-02-15 11:10:54]

Still looks good.

http://codereview.chromium.org/606017/diff/4001/3005
File test/mjsunit/array-shift.js (right):

http://codereview.chromium.org/606017/diff/4001/3005#newcode45
test/mjsunit/array-shift.js:45: assertEquals(array[7], Array.prototype[7]);
I guess after shifting array[6] is interesting as well?

[antonm, 2010-02-15 11:30:46]

Sure, please, have a look at http://codereview.chromium.org/596116/show

I'll add corresponding test for Array.unshift as well.

On 2010/02/15 11:10:54, Mads Ager wrote:
> Still looks good.
> 
> http://codereview.chromium.org/606017/diff/4001/3005
> File test/mjsunit/array-shift.js (right):
> 
> http://codereview.chromium.org/606017/diff/4001/3005#newcode45
> test/mjsunit/array-shift.js:45: assertEquals(array[7], Array.prototype[7]);
> I guess after shifting array[6] is interesting as well?