LGTM if nits are addressed and test cases extended.
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.cc#newcode468 src/arm/lithium-codegen-arm.cc:468: int* args_index, Nit: args_foo => arguments_foo for consistency here and at similar places. https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.h File src/arm/lithium-codegen-arm.h (right): https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.h#newcode152 src/arm/lithium-codegen-arm.h:152: int* arguments_index, I think later it will be very hard to understand what arguments_index actually is, so we should document this somehow, but I am not sure where. Perhaps arguments_offset might be a better name? I'll leave this up to you, but we need some kind of comment in any case. https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#newcode679 src/deoptimizer.cc:679: // Materialize all heap numbers before looking at arguments. Describe why this is necessary to do before arguments objects materialization. https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#newcode718 src/deoptimizer.cc:718: if (!frame->has_adapted_arguments()) { Can rearrange things a bit here so that we have a single 'if'? if (frame->has_adapted_arguments()) { ... } else { ... } Seeing the different cases is a bit hard currently, but this might be due to the inherent complexity of the problem. :-) https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regress/regress-2261.js File test/mjsunit/regress/regress-2261.js (right): https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regress/regress-2261.js#newcode32 test/mjsunit/regress/regress-2261.js:32: As was dicussed already offline, it would be good to have test cases which involve an arguments adaptor (both with too few/to many arguments), even if it is not easily possible to do without extensive copy-n-paste. I would very much prefer a broader test coverage for such a tricky area over copy-n-paste-free code. :-) Probably these tests should live in mjsunit/compiler then. https://chromiumcodereview.appspot.com/10908194/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
