Addresses comments. + Factored out common code sequence "Move TOS -> expression context"
http://codereview.chromium.org/496009/diff/1/5 File src/compiler.cc (right): http://codereview.chromium.org/496009/diff/1/5#newcode1028 src/compiler.cc:1028: BAILOUT("Lookup slot"); On 2009/12/14 17:06:24, Kevin Millikin wrote: > Maybe change the string to mention count operation, so it doesn't overlap with > another bailout string. Done. http://codereview.chromium.org/496009/diff/1/5#newcode1032 src/compiler.cc:1032: ASSERT(prop->obj()->context() == Expression::kUninitialized || On 2009/12/14 17:06:24, Kevin Millikin wrote: > This assert is actually in ProcessExpression, so we don't need it here. Done. I also removed the redundandt ASSERT from a couple of other places. http://codereview.chromium.org/496009/diff/1/5#newcode1051 src/compiler.cc:1051: BAILOUT("non-variable/non-property assignment"); On 2009/12/14 17:06:24, Kevin Millikin wrote: > Change this string too. Done. http://codereview.chromium.org/496009/diff/1/3 File src/fast-codegen.h (right): http://codereview.chromium.org/496009/diff/1/3#newcode254 src/fast-codegen.h:254: // Load a value from a named property and push the result on the stack. On 2009/12/14 17:06:24, Kevin Millikin wrote: > "named" ==> "keyed" Done. http://codereview.chromium.org/496009/diff/1/4 File src/ia32/fast-codegen-ia32.cc (right): http://codereview.chromium.org/496009/diff/1/4#newcode1403 src/ia32/fast-codegen-ia32.cc:1403: switch (assign_type) { On 2009/12/14 17:06:24, Kevin Millikin wrote: > Maybe remove the repeated code with: > if (assign_type == VARIABLE) { > EmitVariableLoad(...); > } else { > // Reserve space for result of postfix operation. > Visit(prop->obj()); > if (assign_type == NAMED_PROPERTY) { > EmitNamedPropertyLoad(...); > } else { > Visit(prop->key(); > EmitKeyedPropertyLoad(...); > } > } Done. http://codereview.chromium.org/496009/diff/1/4#newcode1479 src/ia32/fast-codegen-ia32.cc:1479: EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(), On 2009/12/14 17:06:24, Kevin Millikin wrote: > It means repeating the call to EmitVariableAssignment (but avoids testing > is_postfix twice). I like: > if (expr->is_postfix()) { > EmitVariableAssignment(..., Expression::kEffect); > // Postfix-specific handling. > } else { > EmitVariableAssignment(..., expr->context()); > } Done. http://codereview.chromium.org/496009/diff/1/4#newcode1525 src/ia32/fast-codegen-ia32.cc:1525: __ Drop(1); // Result in on the stack under the receiver. On 2009/12/14 17:06:24, Kevin Millikin wrote: > "in" ==> "is" Done. http://codereview.chromium.org/496009/diff/1/4#newcode1526 src/ia32/fast-codegen-ia32.cc:1526: if (expr->context() != Expression::kEffect) { On 2009/12/14 17:06:24, Kevin Millikin wrote: > You might consider > if (expr->context() != Expression::kEffect && > expr->context() != Expression::kValue) { ... } > It matches what you do for variables, and avoids pop/push for value contexts. Agreed. I introduced a new common code sequence MoveTOS which moves the top of stack into a destination context. http://codereview.chromium.org/496009/diff/1/4#newcode1542 src/ia32/fast-codegen-ia32.cc:1542: if (expr->context() != Expression::kEffect) { On 2009/12/14 17:06:24, Kevin Millikin wrote: > Same comment. Done. http://codereview.chromium.org/496009 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
