A bunch of style comments in the first round.
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)) { 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. http://codereview.chromium.org/302002/diff/4001/4004#newcode466 Line 466: if (FLAG_trace_bailout) PrintF("Unsupported literal\n"); Two space indent. http://codereview.chromium.org/302002/diff/4001/4004#newcode494 Line 494: bool CodeGenSelector::CheckDeclarations(FunctionLiteral* fun) { 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; } } http://codereview.chromium.org/302002/diff/4001/4004#newcode517 Line 517: void CodeGenSelector::VisitDeclaration(Declaration* decl) { 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"); } } http://codereview.chromium.org/302002/diff/4001/4004#newcode734 Line 734: BAILOUT("CallRuntime"); 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. 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() { There is an AstVisitor::VisitDeclarations that you can override. http://codereview.chromium.org/302002/diff/4001/4005#newcode102 Line 102: if ((slot != NULL && slot->type() == Slot::LOOKUP) || !var->is_global()) { 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? 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 { 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. 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"); 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. http://codereview.chromium.org/302002/diff/4001/4003#newcode122 Line 122: // Push function name. This comment seems superfluous. http://codereview.chromium.org/302002/diff/4001/4003#newcode124 Line 124: // Push global object (receiver) Period at the end of comment. http://codereview.chromium.org/302002/diff/4001/4003#newcode126 Line 126: // Push the arguments. This comment is probably unnecessary. http://codereview.chromium.org/302002/diff/4001/4003#newcode211 Line 211: // Push the boilerplate on the stack. Comment doesn't seem necessary. Instead, "// Create a new closure." belongs here not below. http://codereview.chromium.org/302002 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
