Please take another look
http://codereview.chromium.org/8820014/diff/4001/src/arm/builtins-arm.cc File src/arm/builtins-arm.cc (right): http://codereview.chromium.org/8820014/diff/4001/src/arm/builtins-arm.cc#newcode397 src/arm/builtins-arm.cc:397: Label bailout; On 2011/12/07 17:02:47, Jakob wrote:
Unused label. (Does this even compile with -Werror?)
Done. http://codereview.chromium.org/8820014/diff/4001/src/arm/builtins-arm.cc#newcode398 src/arm/builtins-arm.cc:398: __ mov(r7, sp); I didn't see it initially, either, and it cost me about an hour. MemOperand(r7, kPointerSize, PostIndex) below changes r7. On 2011/12/07 17:02:47, Jakob wrote:
Why? I don't see how sp would get overwritten.
http://codereview.chromium.org/8820014/diff/4001/src/builtins.cc File src/builtins.cc (right): http://codereview.chromium.org/8820014/diff/4001/src/builtins.cc#newcode238 src/builtins.cc:238: array->set_length(Smi::FromInt(0)); It's actually unneeded. Removed. On 2011/12/07 17:02:47, Jakob wrote:
A comment explaining why the length must be set to 0 first would be
nice. http://codereview.chromium.org/8820014/diff/4001/src/builtins.cc#newcode255 src/builtins.cc:255: FixedArrayBase* elms = FixedArrayBase::cast(obj); On 2011/12/07 17:02:47, Jakob wrote:
Thanks to templates, you can clean this up a little by writing: if (!maybe_obj->To(&elms)) return maybe_obj; Then you wouldn't need "Object* obj;" at all. I don't mind either way,
your
call.
Done. http://codereview.chromium.org/8820014/diff/4001/src/ia32/builtins-ia32.cc File src/ia32/builtins-ia32.cc (right): http://codereview.chromium.org/8820014/diff/4001/src/ia32/builtins-ia32.cc#newcode1246 src/ia32/builtins-ia32.cc:1246: // esp[4] : argc On 2011/12/07 17:02:47, Jakob wrote:
nit: excess space
Done. http://codereview.chromium.org/8820014/diff/4001/src/ia32/builtins-ia32.cc#newcode1265 src/ia32/builtins-ia32.cc:1265: // esp[4] : argc On 2011/12/07 17:02:47, Jakob wrote:
same nit here
Done. http://codereview.chromium.org/8820014/diff/4001/src/ia32/builtins-ia32.cc#newcode1286 src/ia32/builtins-ia32.cc:1286: // esp[4] : argc On 2011/12/07 17:02:47, Jakob wrote:
same nit here
Done. http://codereview.chromium.org/8820014/diff/4001/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/8820014/diff/4001/src/objects-inl.h#newcode1282 src/objects-inl.h:1282: mode); On 2011/12/07 17:02:47, Jakob wrote:
nit: no need for line break
Done. http://codereview.chromium.org/8820014/diff/4001/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8820014/diff/4001/src/objects.cc#newcode8370 src/objects.cc:8370: void JSArray::Expand(int required_size) { On 2011/12/07 17:02:47, Jakob wrote:
Instead of all this code here, we should be able to make use of SetFastElementsCapacityAndLength (passing in the original length, and required_size as capacity).
Done. http://codereview.chromium.org/8820014/diff/4001/test/mjsunit/array-construct-transition.js File test/mjsunit/array-construct-transition.js (right): http://codereview.chromium.org/8820014/diff/4001/test/mjsunit/array-construct-transition.js#newcode33 test/mjsunit/array-construct-transition.js:33: a = new Array(0, 1, 2); On 2011/12/07 17:02:47, Jakob wrote:
nit: "var a", please. Same for b and c.
Done. http://codereview.chromium.org/8820014/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
