Still debugging - not ready for final review.
http://codereview.chromium.org/21538/diff/2001/2003 File src/codegen-ia32.cc (right): http://codereview.chromium.org/21538/diff/2001/2003#newcode753 Line 753: Result GenerateInlineCode(Result* left, Result* right); On 2009/02/23 09:10:16, Kevin Millikin wrote: > This needs a comment both here and at the implementation telling whether the > results "left" and "right" are consumed. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode833 Line 833: if (!right_const_smi) { On 2009/02/23 09:10:16, Kevin Millikin wrote: > I'd rather not see negated conditions when there is both a then and else part. > It seems more straightforward to swap them. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode929 Line 929: if (answer_object == Heap::undefined_value_) { On 2009/02/23 09:10:16, Kevin Millikin wrote: > Use the accessor Heap::undefined_value(). Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode932 Line 932: Result answer_result(Handle<Object *>(answer_object), this); On 2009/02/23 09:10:16, Kevin Millikin wrote: > You mean Handle<Object>? No space between "Object" and "*". Don't we have a > VirtualFrame::Push member function that takes a handle anyway, so there's no > need to build an result. > I think that explicitly constructing a Result object should be treated as a bad > smell. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5438 Line 5438: __ mov(answer.reg(), Operand(eax)); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Use the register-register encoding of mov, not the register-operand one. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5467 Line 5467: __ mov(answer.reg(), Operand(edx)); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Ditto. Done. http://codereview.chromium.org/21538/diff/2001/2003#newcode5477 Line 5477: __ mov(eax, Operand(left.reg())); On 2009/02/23 09:10:16, Kevin Millikin wrote: > Ditto. Done. http://codereview.chromium.org/21538/diff/2001/2002 File src/codegen-ia32.h (right): http://codereview.chromium.org/21538/diff/2001/2002#newcode483 Line 483: void SmiOperation(Result* operand, On 2009/02/23 09:10:16, Kevin Millikin wrote: > This needs a comment in the header telling whether it consumes the result > "operand" or not. Done. http://codereview.chromium.org/21538 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
