I addressed the comments. Added x64 and arm code.
http://codereview.chromium.org/302002/diff/4001/4004 File src/compiler.cc (right): http://codereview.chromium.org/302002/diff/4001/4004#newcode461 Line 461: if (!CheckDeclarations(fun)) { On 2009/10/19 19:50:27, Kevin Millikin wrote: > You can make this function set the has_supported_syntax flag and do: > has_supported_syntax_ = true; // Here not below. > VisitDeclarations(fun->scope()->declarations()); // Renamed, see next comment. > if (!has_supported_syntax_) return NORMAL; > Also, it might be good to put the fast checks (like materialized literals) first > to get an early cheap bailout for now. Agreed. DONE. http://codereview.chromium.org/302002/diff/4001/4004#newcode466 Line 466: if (FLAG_trace_bailout) PrintF("Unsupported literal\n"); On 2009/10/19 19:50:27, Kevin Millikin wrote: > Two space indent. DONE. http://codereview.chromium.org/302002/diff/4001/4004#newcode494 Line 494: bool CodeGenSelector::CheckDeclarations(FunctionLiteral* fun) { On 2009/10/19 19:50:27, Kevin Millikin wrote: > Here you can override AstVisitor::VisitDeclarations: > void CodeGenSelector::VisitDeclarations(ZoneList<Declaration*>* decls) { > for (int i = 0; i < decls->length(); i++) { > Visit(decls->at(i)); > CHECK_BAILOUT; > } > } DONE. http://codereview.chromium.org/302002/diff/4001/4004#newcode517 Line 517: void CodeGenSelector::VisitDeclaration(Declaration* decl) { On 2009/10/19 19:50:27, Kevin Millikin wrote: > And here you can provide a definition: > void CodeGenSelector::VisitDeclaration(Declaration* decl) { > Variable* var = decl->proxy()->var(); > if (!var->is_global() || var->mode() == Variable::CONST) { > BAILOUT("reason"); > } > } DONE. http://codereview.chromium.org/302002/diff/4001/4004#newcode734 Line 734: BAILOUT("CallRuntime"); On 2009/10/19 19:50:27, Kevin Millikin wrote: > The rule we use is that the body does not have braces surrounding it if and only > if it is on the same line as the if. > This one probably fits on the same line with the if. DONE. http://codereview.chromium.org/302002/diff/4001/4005 File src/fast-codegen.cc (right): http://codereview.chromium.org/302002/diff/4001/4005#newcode72 Line 72: void FastCodeGenerator::ProcessDeclarations() { On 2009/10/19 19:50:27, Kevin Millikin wrote: > There is an AstVisitor::VisitDeclarations that you can override. Yes, but the function ProcessDeclarations traverses the declarations list twice. I just copied the old code generators function here. But we may refactor it later. http://codereview.chromium.org/302002/diff/4001/4005#newcode102 Line 102: if ((slot != NULL && slot->type() == Slot::LOOKUP) || !var->is_global()) { On 2009/10/19 19:50:27, Kevin Millikin wrote: > I've never really liked this if with an empty body, even though it parallels > exactly the loop above. What do you think of: > if ((slot == NULL || slot->type() == Slot::LOOKUP) && var->is_global()) { ... > and then no else? Agreed. I guess it should be: if ((slot == NULL || slot->type() != Slot::LOOKUP) && var->is_global()) {... then. http://codereview.chromium.org/302002/diff/4001/4002 File src/fast-codegen.h (right): http://codereview.chromium.org/302002/diff/4001/4002#newcode64 Line 64: Operand ContextOperand(Register context, int index) const { On 2009/10/19 19:50:27, Kevin Millikin wrote: > This function and the next are platform-specific. The type 'Operand' is not > correct on all platforms---it should be MemOperand on ARM, and the next function > specifically mentions register esi. > I think I'd prefer to keep them out of here for now. We should be able to make > the corresponding CodeGenerator functions static and call them for now at the > cost of writing CodeGenerator:: at the call sites. > It would be nice if we could find a way to keep a single header file interface, > perhaps by adding a platform specific CodeGenHelper or something. Yes, agreed. I moved ContextOperand to ia32/fastcodegen-ia32.cc as a static function for now. http://codereview.chromium.org/302002/diff/4001/4003 File src/ia32/fast-codegen-ia32.cc (right): http://codereview.chromium.org/302002/diff/4001/4003#newcode67 Line 67: Comment cmnt(masm_, "[ declarations"); On 2009/10/19 19:50:27, Kevin Millikin wrote: > Consider moving the comment into VisitDeclarations. If not, it should have a > block scope (a) so there isn't a problem with another identifier named cmnt in > the same scope and (b) because the Comment class will print a matching "]" when > the destructor is run. DONE. http://codereview.chromium.org/302002/diff/4001/4003#newcode122 Line 122: // Push function name. On 2009/10/19 19:50:27, Kevin Millikin wrote: > This comment seems superfluous. Done. http://codereview.chromium.org/302002/diff/4001/4003#newcode124 Line 124: // Push global object (receiver) On 2009/10/19 19:50:27, Kevin Millikin wrote: > Period at the end of comment. Done. http://codereview.chromium.org/302002/diff/4001/4003#newcode126 Line 126: // Push the arguments. On 2009/10/19 19:50:27, Kevin Millikin wrote: > This comment is probably unnecessary. Done. http://codereview.chromium.org/302002/diff/4001/4003#newcode211 Line 211: // Push the boilerplate on the stack. On 2009/10/19 19:50:27, Kevin Millikin wrote: > Comment doesn't seem necessary. Instead, "// Create a new closure." belongs > here not below. Done. http://codereview.chromium.org/302002 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
