http://codereview.chromium.org/998001/diff/18009/25007 File src/ast.cc (right):
http://codereview.chromium.org/998001/diff/18009/25007#newcode598 src/ast.cc:598: // Ast node number should be unique. Assert that we don't copy as nodes that In the comment text: "Ast" ==> "AST", "as" ==> "AST". http://codereview.chromium.org/998001/diff/18009/25004 File src/ast.h (right): http://codereview.chromium.org/998001/diff/18009/25004#newcode357 src/ast.h:357: Block(Block* other, ZoneList<Statement*>* statements); It's not obvious what this will do (why does it need both a list of statements and another block?). It needs a short comment here. http://codereview.chromium.org/998001/diff/18009/25004#newcode423 src/ast.h:423: IterationStatement(IterationStatement* other, Statement* body); Not obvious what this will do. Needs a short comment. http://codereview.chromium.org/998001/diff/18009/25004#newcode501 src/ast.h:501: ForStatement(ForStatement* other, Not obvious what this will do. http://codereview.chromium.org/998001/diff/18009/25004#newcode578 src/ast.h:578: ExpressionStatement(ExpressionStatement* other, Expression* expression); Not obvious. http://codereview.chromium.org/998001/diff/18009/25004#newcode722 src/ast.h:722: IfStatement(IfStatement* other, Same. http://codereview.chromium.org/998001/diff/18009/25005 File src/compiler.cc (right): http://codereview.chromium.org/998001/diff/18009/25005#newcode108 src/compiler.cc:108: builder.graph()->PrintText(builder.postorder(), function); Please swap the order of the arguments here. http://codereview.chromium.org/998001/diff/18009/25001 File src/data-flow.cc (right): http://codereview.chromium.org/998001/diff/18009/25001#newcode183 src/data-flow.cc:183: ASSERT(stmt->AsForStatement() != NULL); Argument type should just be ForStatement*. http://codereview.chromium.org/998001/diff/18009/25001#newcode188 src/data-flow.cc:188: ForStatement* for_stmt = copy_visitor.DeepCopyStmt(stmt)->AsForStatement(); Why copy the original and then re-copy the test, body and update? Why not just copy the parts you need copies of (test, body and update)? http://codereview.chromium.org/998001/diff/18009/25001#newcode205 src/data-flow.cc:205: graph_.AppendInstruction(init); I don't think you should be building the flow graph here---the code for flow graph building should not be duplicated this way. This function should just return a new statement. In fact, it doesn't need to be a member of FlowGraphBuilder at all, but can be a static function in this file. http://codereview.chromium.org/998001/diff/18009/25001#newcode274 src/data-flow.cc:274: (*stmts)[i] = ProcessStatement((*stmts)[i]); You can write (*stmts)[i] as stmts->at(i), which seems more common in the codebase. http://codereview.chromium.org/998001/diff/18009/25001#newcode283 src/data-flow.cc:283: return PeelForLoop(stmt); If you don't try to build the graph it PeelForLoop, you have: if (FLAG_loop_peeling etc....) { stmt = PeelForLoop(stmt->AsForStatement()); } Visit(stmt); return stmt; http://codereview.chromium.org/998001 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
