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
http://codereview.chromium.org/606017
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
