LGTM.
https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): https://chromiumcodereview.appspot.com/11528003/diff/2001/src/arm/macro-assembler-arm.h#newcode111 src/arm/macro-assembler-arm.h:111: void IterateCompiledFrame(const StandardFrame* frame, ObjectVisitor* v); On 2012/12/11 07:59:06, Sven Panne wrote:
Shouldn't this be a method of StandardFrame? A naked top-level
function always
looks a bit suspicious in an OO world...
Same for other platforms.
Well, the name "IterateCompiledFrame" expresses the notion that this is how you iterate over a compiled frame, not over a StandardFrame, so renaming this to StandardFrame::Iterate would not be appropriate (e.g. because we have InternalFrame which also is-a StandardFrame but iterates differently). I'm not opposed to moving it into the class without renaming it (i.e. calling it StandardFrame::IterateCompiledFrame), but I don't feel strongly about it. https://chromiumcodereview.appspot.com/11528003/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/11528003/diff/2001/src/hydrogen-instructions.h#newcode1947 src/hydrogen-instructions.h:1947: SetOperandAt(1, typecheck != NULL ? typecheck : value); I could swear I've reviewed this change before... ah, yes: https://codereview.chromium.org/11464027/ https://chromiumcodereview.appspot.com/11528003/diff/2001/src/ia32/macro-assembler-ia32.h File src/ia32/macro-assembler-ia32.h (right): https://chromiumcodereview.appspot.com/11528003/diff/2001/src/ia32/macro-assembler-ia32.h#newcode61 src/ia32/macro-assembler-ia32.h:61: void IterateCompiledFrame(const StandardFrame* frame, ObjectVisitor* v); We #include frames.h above, which contains an identical declaration (except for "const"). Can you get rid of this line by moving the constness into the declaration in frames.h? Same for the other platforms. https://chromiumcodereview.appspot.com/11528003/diff/2001/src/x64/lithium-codegen-x64.cc File src/x64/lithium-codegen-x64.cc (right): https://chromiumcodereview.appspot.com/11528003/diff/2001/src/x64/lithium-codegen-x64.cc#newcode310 src/x64/lithium-codegen-x64.cc:310: __ push(rsi); oops :-) https://chromiumcodereview.appspot.com/11528003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
