This approach seems good. Comments below.
http://codereview.chromium.org/3449004/diff/1/2 File src/ast.h (right): http://codereview.chromium.org/3449004/diff/1/2#newcode181 src/ast.h:181: // Evaluated for its value (and side effects). Result on stack. The distinction between Stack/Accumulator is specific to a particular AST visitor (the FullCodeGenerator), so maybe this whole enum should be moved out of Expression and into FullCodeGenerator::Context. http://codereview.chromium.org/3449004/diff/1/2#newcode184 src/ast.h:184: kValueAccumulator, The names should match the classes in the full code generator: kStackValue, kAccumulatorValue. http://codereview.chromium.org/3449004/diff/1/4 File src/full-codegen.h (right): http://codereview.chromium.org/3449004/diff/1/4#newcode74 src/full-codegen.h:74: new_context_(NULL), It's confusing to have both context_ and new_context_ here. Don't they change in lockstep? Can context_ be a member of new_context_ (and rename context_ ==> kind_, new_context_ ==> context_)? http://codereview.chromium.org/3449004/diff/1/4#newcode255 src/full-codegen.h:255: void Apply(Register reg) { new_context_->Apply(reg); } To me, these Apply helpers that don't mention the context are weird to read at the call site. It doesn't hurt to much to just get rid of them and write context()->Apply(eax); or whatever at all the call sites, does it? Maybe the "Apply" name is too cute. "Plug" seems strictly better. http://codereview.chromium.org/3449004/diff/1/4#newcode283 src/full-codegen.h:283: bool ContextIsEffect() { This could be replaced by virtual type testers on the context classes. http://codereview.chromium.org/3449004/diff/1/4#newcode337 src/full-codegen.h:337: Label* saved_true = true_label_; Aren't these labels subsumed by the ones in the expression context? http://codereview.chromium.org/3449004/diff/1/4#newcode491 src/full-codegen.h:491: ExpressionContext* new_context() { return new_context_; } Can this be const? http://codereview.chromium.org/3449004/diff/1/4#newcode546 src/full-codegen.h:546: // Convert constant control flow (true or false) to the result expected for Rewrite all these comments so they don't mention "a given" context, because the receiver is not so given. http://codereview.chromium.org/3449004/diff/1/5 File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/3449004/diff/1/5#newcode1392 src/ia32/full-codegen-ia32.cc:1392: EmitNamedPropertyLoad(property); You also want to visit the named (and keyed below) properties in an AccumulatorValue context, not in whatever the outer context of the assignment happens to be. Even if they don't rely on the specific context (or assume AccumulatorValue), I think it's safer to widen the scope of AccumulatorValueContext to include these calls. http://codereview.chromium.org/3449004/diff/1/5#newcode3428 src/ia32/full-codegen-ia32.cc:3428: if (context == Expression::kValueStack) { This reads a bit better if you have something like: bool should_push = (context == Expression::kValueStack); ASSERT(should_push || context == Expression::kValueAccumulator); up front and then: if (should_push) __ push(eax); At all push sites in this function. http://codereview.chromium.org/3449004/diff/1/5#newcode3456 src/ia32/full-codegen-ia32.cc:3456: if (context == Expression::kValueAccumulator) { It seems legitimate to change the calls to VisitForTypeofValue to have an explicit ExpressionContext pushed instead of passing a token as argument: { StackValueContext ctxt(this); VisitForTypeofValue(expr->expression()); } Then here in this case you can have a "tail call" that forwards to Visit in the current context with just Visit(expr). http://codereview.chromium.org/3449004/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
