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.
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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.
This code is now gone.
http://codereview.chromium.org/3449004/diff/1/2#newcode184
src/ast.h:184: kValueAccumulator,
On 2010/09/15 09:04:08, Kevin Millikin wrote:
The names should match the classes in the full code generator:
kStackValue,
kAccumulatorValue.
This code is now gone.
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),
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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_)?
That was an intermediate stage. We now have only new_context and it is
called context
http://codereview.chromium.org/3449004/diff/1/4#newcode255
src/full-codegen.h:255: void Apply(Register reg) {
new_context_->Apply(reg); }
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/3449004/diff/1/4#newcode283
src/full-codegen.h:283: bool ContextIsEffect() {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
This could be replaced by virtual type testers on the context classes.
Done.
http://codereview.chromium.org/3449004/diff/1/4#newcode337
src/full-codegen.h:337: Label* saved_true = true_label_;
On 2010/09/15 09:04:08, Kevin Millikin wrote:
Aren't these labels subsumed by the ones in the expression context?
Done.
http://codereview.chromium.org/3449004/diff/1/4#newcode491
src/full-codegen.h:491: ExpressionContext* new_context() { return
new_context_; }
On 2010/09/15 09:04:08, Kevin Millikin wrote:
Can this be const?
Done.
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
On 2010/09/15 09:04:08, Kevin Millikin wrote:
Rewrite all these comments so they don't mention "a given" context,
because the
receiver is not so given.
Done.
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);
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/3449004/diff/1/5#newcode3428
src/ia32/full-codegen-ia32.cc:3428: if (context ==
Expression::kValueStack) {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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.
Actually after the change below this just turns into
context()->Apply(eax) and I can get rid of the if around the push.
http://codereview.chromium.org/3449004/diff/1/5#newcode3456
src/ia32/full-codegen-ia32.cc:3456: if (context ==
Expression::kValueAccumulator) {
On 2010/09/15 09:04:08, Kevin Millikin wrote:
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).
Done.
http://codereview.chromium.org/3449004/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
