LGTM, with comments.
http://codereview.chromium.org/155047/diff/1/8 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/155047/diff/1/8#newcode779 Line 779: PrintF("GenericBinaryOpStub (%s)\n", Token::String(op_)); Why not make this message more informative, by including known_rhs_? http://codereview.chromium.org/155047/diff/1/8#newcode792 Line 792: int known_rhs) { Would "constant_rhs" be a better name than "known_rhs"? http://codereview.chromium.org/155047/diff/1/8#newcode925 Line 925: if (popcnt > 2) return false; The whole function can be replaced by: x &= x - 1; return (x & (x-1)) != 0; http://codereview.chromium.org/155047/diff/1/8#newcode1045 Line 1045: case Token::SHL: { Why no check of shift_value with 0 for SHL? If it is ok, comment it. http://codereview.chromium.org/155047/diff/1/8#newcode5477 Line 5477: __ mov(destination, Operand(destination, LSL, first_bit)); Are you going to find out if the value overflowed from the flags? Will they be set right from this combination of add and move? http://codereview.chromium.org/155047/diff/1/8#newcode5496 Line 5496: int* result_needs_shifting) { // Including Smi tag shift I would call this shift_amount or required_shift. result_needs_shifting sounds like a boolean. http://codereview.chromium.org/155047 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
