Initial round of comments.
http://codereview.chromium.org/486008/diff/1/4 File src/ast.h (right): http://codereview.chromium.org/486008/diff/1/4#newcode1245 src/ast.h:1245: return op() != Token::ASSIGN && op() != Token::INIT_VAR; } Don't forget Token::INIT_CONST. There is already a dependence on the order of tokens in a few places, so you could just write: return op() > Token::ASSIGN and add a comment in token.h that Assignment::is_compound() also depends on the order of tokens. http://codereview.chromium.org/486008/diff/1/3 File src/ia32/fast-codegen-ia32.cc (right): http://codereview.chromium.org/486008/diff/1/3#newcode380 src/ia32/fast-codegen-ia32.cc:380: void FastCodeGenerator::Drop(int drop_count) { There's already a MacroAssembler::Drop, you could just use that. http://codereview.chromium.org/486008/diff/1/3#newcode557 src/ia32/fast-codegen-ia32.cc:557: EmitVariableLoad(expr); Why not pass the expression context to EmitVariableLoad, so it can avoid add(esp, 4); push(eax) for global variable accesses in value context? http://codereview.chromium.org/486008/diff/1/3#newcode562 src/ia32/fast-codegen-ia32.cc:562: void FastCodeGenerator::EmitVariableLoad(VariableProxy* expr) { It might make more sense for this to take a Variable* instead of a VariableProxy*. http://codereview.chromium.org/486008/diff/1/3#newcode848 src/ia32/fast-codegen-ia32.cc:848: void FastCodeGenerator::EmitVariableLoadOnStack(Assignment* expr) { It might make more sense for this to take a Variable* instead of an Assignment*. http://codereview.chromium.org/486008/diff/1/3#newcode855 src/ia32/fast-codegen-ia32.cc:855: void FastCodeGenerator::EmitNamedPropertyLoadOnStack(Assignment* expr) { It might make more sense for this to take a Property* (or even, Expression* object and Handle<String> name). http://codereview.chromium.org/486008/diff/1/3#newcode864 src/ia32/fast-codegen-ia32.cc:864: void FastCodeGenerator::EmitKeyedPropertyLoadOnStack(Assignment* expr) { Ditto. http://codereview.chromium.org/486008/diff/1/3#newcode871 src/ia32/fast-codegen-ia32.cc:871: void FastCodeGenerator::EmitCompoundAssignmentOp(Assignment* expr) { It might make more sense for this to just take the Token::Value. http://codereview.chromium.org/486008/diff/1/3#newcode876 src/ia32/fast-codegen-ia32.cc:876: __ push(eax); Don't we actually want the value in the accumulator here (we'll pop it before the store, right?)? http://codereview.chromium.org/486008 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
