Next round of comments.
http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1410 src/arm/macro-assembler-arm.cc:1410: void MacroAssembler::EnterExitFramePrologue(int unwind_space) { Thanks a lot for introducing this macros. But you still keep the code duplicated (see ::EnterExitFrame above) http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1412 src/arm/macro-assembler-arm.cc:1412: stm(db_w, sp, fp.bit() | ip.bit() | lr.bit()); Please, retain the original comment which describes the semantics of this push http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1426 src/arm/macro-assembler-arm.cc:1426: // Create space for the args nit: period at the end of the comment, please http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1433 src/arm/macro-assembler-arm.cc:1433: tst(sp, Operand(frame_alignment_mask)); please, keep the comment http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1471 src/arm/macro-assembler-arm.cc:1471: // patch the exit frame pc and Call the api function! nit: [P]atch and [c]all http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1481 src/arm/macro-assembler-arm.cc:1481: // otherwise set it to undefined nit: period at the end of the comment. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1482 src/arm/macro-assembler-arm.cc:1482: LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); it looks like you forgot cmp(r0, Operand(0)) http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc#newcode1501 src/arm/macro-assembler-arm.cc:1501: mov(ip, Operand(ExternalReference::the_hole_value_location())); please, use LoadRoot. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h#newcode646 src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. nit: we use |<parameter name>| convention. Ditto for unwind_space below http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h#newcode646 src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. maybe some other name? allocation_size or something like that? http://codereview.chromium.org/6170001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
