Uploaded new version with comments addressed.
I'm still processing expressions left-to-right (and saving the intermediate
result of the left). Are we sure that we never need the set of assigned
variables of a left sub-tree?
If we need this information it does not make sense to change the traversal
order
now and change it back later.
http://codereview.chromium.org/669155/diff/32/1042
File src/ast.cc (right):
http://codereview.chromium.org/669155/diff/32/1042#newcode81
src/ast.cc:81: bool VariableProxy::IsLocal() {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
This seems misleading. I think of a variable "IsLocal" if it is in
the function
body scope. This predicate tests is local and is stack allocated.
Done.
http://codereview.chromium.org/669155/diff/32/1039
File src/ast.h (right):
http://codereview.chromium.org/669155/diff/32/1039#newcode1029
src/ast.h:1029: bool IsParam();
On 2010/03/08 10:55:32, Kevin Millikin wrote:
IsParameter.
Done.
http://codereview.chromium.org/669155/diff/32/1032
File src/data-flow.cc (right):
http://codereview.chromium.org/669155/diff/32/1032#newcode584
src/data-flow.cc:584: if (stmt->init()->AsExpressionStatement() != NULL
&&
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Since this is a syntactic property, I think it's fair to make it a
virtual
function on AstNode. That way, you avoid all the type testing here.
Assignment* init = stmt->init()->AsSimpleAssignment();
where
Assignment* ExpressionStatement::AsSimpleAssignent() {
return expression()->AsSimpleAssignment();
}
Assignment* Block::AsSimpleAssignment() {
if (statements_.length() != 1) return NULL;
return statements_[0]->AsSimpleAssignment();
}
Assignment* Assignment::AsSimpleAssignment() {
return this;
}
Done. Good idea.
http://codereview.chromium.org/669155/diff/32/1032#newcode601
src/data-flow.cc:601: if (init->target()->AsVariableProxy() == NULL)
return NULL;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
I'm concerned that you don't seem to check the operator in the
assignment
expression. What if it's += for instance?
This reads better if you write:
VariableProxy* proxy = init->target()->AsVariableProxy();
if (proxy == NULL || !proxy->IsLocal()) return NULL;
Done.
http://codereview.chromium.org/669155/diff/32/1032#newcode609
src/data-flow.cc:609: if (init->value()->AsLiteral() == NULL ||
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Likewise:
Literal* value = init->value()->AsLiteral();
if (value == NULL || !value->handle()->IsSmi()) return NULL;
int init_value = Smi::cast(*value->handle())->value();
Done.
http://codereview.chromium.org/669155/diff/32/1032#newcode650
src/data-flow.cc:650: if (init_value > term_value && update->op() !=
Token::DEC) return NULL;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Does this handle all the possibilities? What about 'for (var i = 0; i
0;
i++)'?
Right. I need to check for the case where bound1 == bound2.
For now I think I'll accept missing the cases where the loop is never
entered or executed once because those should not hurt. Otherwise the
condition would be quite complicated.
http://codereview.chromium.org/669155/diff/32/1032#newcode657
src/data-flow.cc:657: int AssignedVariablesAnalyzer::BitIndex(Variable*
var) {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Extra space after 'int'. Maybe it's premature, but it seems like this
function
should be inlined.
Done.
http://codereview.chromium.org/669155/diff/32/1032#newcode683
src/data-flow.cc:683: void
AssignedVariablesAnalyzer::VisitStatements(ZoneList<Statement*>* stmts)
{
On 2010/03/08 10:55:32, Kevin Millikin wrote:
This is the default implementation, so it doesn't need to be
overridden.
Done.
http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
I expect the call to Union right after this, because it seems like
that's where
you switch from back from collecting to accumulating the assigned
vars.
I could write:
result.Union(av_);
av_.Clear();
here but it seems redundant since I don't need to the set of assigned
variables of the then- and the else-block separately. So I only do
union/clear before visiting expressions (where I also assert that av_ is
empty) and in for-loops where I need the result of from the loop body.
http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
I expect the call to Union right after this, because it seems like
that's where
you switch from back from collecting to accumulating the assigned
vars.
Agree. I'll combine the construction+copying into a copy constructor for
now.
http://codereview.chromium.org/669155/diff/32/1032#newcode716
src/data-flow.cc:716: if (stmt->HasElseStatement()) {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
It seems simpler just to visit the (possibly empty) else statement.
Done.
http://codereview.chromium.org/669155/diff/32/1032#newcode782
src/data-flow.cc:782: Visit(stmt->cond());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Union after this Visit?
As above no need before visiting statements.
http://codereview.chromium.org/669155/diff/32/1032#newcode792
src/data-flow.cc:792: Visit(stmt->cond());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
And here?
As above no need before visiting statements.
http://codereview.chromium.org/669155/diff/32/1032#newcode824
src/data-flow.cc:824: Variable* var = FindSmiLoopVariable(stmt);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
This needs a comment that av_ is the assigned variables in the body.
Done.
http://codereview.chromium.org/669155/diff/32/1041
File src/data-flow.h (right):
http://codereview.chromium.org/669155/diff/32/1041#newcode187
src/data-flow.h:187: class AssignedVariablesAnalyzer : public AstVisitor
{
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Needs some simple comment about what it's doing. E.g., collect
assigned
variables in expressions right-to-left and annotate variables and for
loops....
Done.
http://codereview.chromium.org/669155/diff/32/1041#newcode189
src/data-flow.h:189: explicit AssignedVariablesAnalyzer(FunctionLiteral*
fun);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
I like it better if fun is an argument to Analyze, not the
constructor.
Since the member bitvector av_ is an embedded instance it needs to be
initialized in the constructor with the correct length. I'm not sure if
it is worth having a pointer to a bitvector instead. It would add
another indirection on accessing the bitvector, wouldn't it?
http://codereview.chromium.org/669155/diff/32/1041#newcode194
src/data-flow.h:194: int BitIndex(Variable* var);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
Extra space after int.
Done.
http://codereview.chromium.org/669155/diff/32/1041#newcode206
src/data-flow.h:206: int num_params_;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
If you've got the function literal, you don't need num_params_ or
num_locals_
here---you can just get them from the literal or scope. They only
seem to be
used once or twice anyway.
Most uses of num_vars_ can be replaced with av_.length().
Done.
http://codereview.chromium.org/669155
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
