Looks good. A few comments.
http://codereview.chromium.org/42006/diff/1/3 File src/codegen-ia32.cc (right): http://codereview.chromium.org/42006/diff/1/3#newcode686 Line 686: // should be inlined, placed in the stub, or omitted entirely. To parallel the order of the enum, "should be placed in the stub, inlined, or omitted entirely". http://codereview.chromium.org/42006/diff/1/3#newcode722 Line 722: class OpBits: public BitField<Token::Value, 2, 12> {}; You might consider asserting in the constructor that NUM_TOKENS fits in 12 bits. http://codereview.chromium.org/42006/diff/1/3#newcode834 Line 834: // Compute the result, and return that as a constant on the frame. Maybe should avoid words like "return" because they can be ambiguous (return from this C++ function? return at runtime?). "Compute the result and leave it on the frame."? http://codereview.chromium.org/42006/diff/1/3#newcode845 Line 845: false, overwrite_mode); Wrong indentation? http://codereview.chromium.org/42006/diff/1/3#newcode848 Line 848: ConstantSmiBinaryOperation(op, &right, left.handle(), type, Wrong indentation? http://codereview.chromium.org/42006/diff/1/3#newcode860 Line 860: // The same stub is used for NO_SMI_CODE and SMI_CODE_INLINED. It's probably better to make the stub handle all three values of the enum rather than just two. It's less confusing here and will be probably be easier to change later by just changing the stub. http://codereview.chromium.org/42006/diff/1/3#newcode956 Line 956: // Create a new deferred code object that calls GenericBinaryOpStub It calls a (specialized) instance of a GenericBinaryOpStub, not the GenericBinaryOpStub. (But I don't think you even need to mention that here, because this non-local comment becomes out of date if the deferred code changes). http://codereview.chromium.org/42006/diff/1/3#newcode1177 Line 1177: // Consumes the argument "operand". You might consider asserting valid entry registers here, because this function assumes that register allocation will not fail. http://codereview.chromium.org/42006/diff/1/3#newcode1220 Line 1220: Result answer(this); // Only allocated a new register if reversed. "allocated" -> "allocate" http://codereview.chromium.org/42006/diff/1/3#newcode1267 Line 1267: if (shift_value > 0) { In general it might be a good idea to try to make sure we have tests that hit these special code paths. http://codereview.chromium.org/42006/diff/1/3#newcode5608 Line 5608: // registers). You might consider asserting valid entry registers here. http://codereview.chromium.org/42006/diff/1/3#newcode5620 Line 5620: __ or_(answer.reg(), Operand(right->reg())); (As a future change, we could simplify the smi check code if left and right are the same registers, eg, "x + x" or "y = x; x + y".) http://codereview.chromium.org/42006/diff/1/2 File src/codegen-ia32.h (right): http://codereview.chromium.org/42006/diff/1/2#newcode486 Line 486: // Emit code to perform a binary operation on Blank line between functions in the header file, please. http://codereview.chromium.org/42006/diff/1/2#newcode496 Line 496: // two likely smis. The code to handle smi arguments is produced inline. Weird word wrapping (very short line followed by very long line, makes me wonder why). For consistency, put variable names in double quotes always or never (I don't think they need it). http://codereview.chromium.org/42006 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
