http://codereview.chromium.org/554062/diff/1/2 File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/554062/diff/1/2#newcode744 src/ia32/codegen-ia32.cc:744: enum ArgLocation { It seems like this should be a member of FloatingPointHelper. http://codereview.chromium.org/554062/diff/1/2#newcode7288 src/ia32/codegen-ia32.cc:7288: if (HasArgumentsInRegisters()) { Is this right? What if op_ == Token::ADD or Token::SUB and !HasArgumentsInRegisters(). Won't the heap allocation target &after_alloc_failure on a failure, but the label will never be bound? http://codereview.chromium.org/554062/diff/1/2#newcode7294 src/ia32/codegen-ia32.cc:7294: } Missing break would be a problem (at least dead code generation) if code was added to the fall through or clauses were reordered. Add break or add a "Fall through" comment. (Explicit break is better). http://codereview.chromium.org/554062/diff/1/3 File src/ia32/codegen-ia32.h (right): http://codereview.chromium.org/554062/diff/1/3#newcode707 src/ia32/codegen-ia32.h:707: return ((op_ == Token::ADD) || (op_ == Token::SUB)) || You lost a two-space indent here. This is a complicated comparison, I like the following better: bool ArgsInRegistersSupported() { if (op_ == Token::ADD || op_ == Token::SUB) return true; if (op_ == Token::MUL || op_ == Token::DIV) { return NO_SMI_CODE_IN_STUB; } return false; } You might also rename this to SupportsArgsInRegisters, to parallel HasArgsInRegisters. http://codereview.chromium.org/554062/diff/1/3#newcode718 src/ia32/codegen-ia32.h:718: bool HasArgumentsInRegisters() { return args_in_registers_; } I'm not sure why HasArguments is spelled out but SetArgs is not. It should be consistent and I don't mind abbreviating Args. You could change it since you're already modifying this code. http://codereview.chromium.org/554062 -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev
