Addressed comments. Introducing an accumulator register for passing intermediate results may be better done as a separate change.
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; } On 2009/12/10 15:31:39, Kevin Millikin wrote: > 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. Done. 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) { On 2009/12/10 15:31:39, Kevin Millikin wrote: > There's already a MacroAssembler::Drop, you could just use that. Done. http://codereview.chromium.org/486008/diff/1/3#newcode557 src/ia32/fast-codegen-ia32.cc:557: EmitVariableLoad(expr); On 2009/12/10 15:31:39, Kevin Millikin wrote: > Why not pass the expression context to EmitVariableLoad, so it can avoid > add(esp, 4); push(eax) for global variable accesses in value context? Agreed. We just have to rely on the kValue context being 'stack'. http://codereview.chromium.org/486008/diff/1/3#newcode562 src/ia32/fast-codegen-ia32.cc:562: void FastCodeGenerator::EmitVariableLoad(VariableProxy* expr) { On 2009/12/10 15:31:39, Kevin Millikin wrote: > It might make more sense for this to take a Variable* instead of a > VariableProxy*. Done. http://codereview.chromium.org/486008/diff/1/3#newcode855 src/ia32/fast-codegen-ia32.cc:855: void FastCodeGenerator::EmitNamedPropertyLoadOnStack(Assignment* expr) { On 2009/12/10 15:31:39, Kevin Millikin wrote: > It might make more sense for this to take a Property* (or even, Expression* > object and Handle<String> name). Done. http://codereview.chromium.org/486008/diff/1/3#newcode864 src/ia32/fast-codegen-ia32.cc:864: void FastCodeGenerator::EmitKeyedPropertyLoadOnStack(Assignment* expr) { On 2009/12/10 15:31:39, Kevin Millikin wrote: > Ditto. Actually I just realized that I don't need any arguments for this one :) http://codereview.chromium.org/486008/diff/1/3#newcode871 src/ia32/fast-codegen-ia32.cc:871: void FastCodeGenerator::EmitCompoundAssignmentOp(Assignment* expr) { On 2009/12/10 15:31:39, Kevin Millikin wrote: > It might make more sense for this to just take the Token::Value. Done. http://codereview.chromium.org/486008/diff/1/3#newcode876 src/ia32/fast-codegen-ia32.cc:876: __ push(eax); On 2009/12/10 15:31:39, Kevin Millikin wrote: > Don't we actually want the value in the accumulator here (we'll pop it before > the store, right?)? The functions for completing the assignment (EmitVariableAssignment, EmitNamedPropertyAssignment,...) currently expect the RHS value on top of the stack. But yes, we could change that, and I think it makes sense. I'd prefer to do this as a separate change. http://codereview.chromium.org/486008 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
