https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h File src/ast.h (right):
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2322 src/ast.h:2322: AstProperties* DetachAstProperties(); I don't understand why the AstProperties ownership is so complicated. Wouldn't it work to: 1. Make AstProperties BASE_EMBEDDED 2. Remove DISALLOW_COPY_AND_ASSIGN(AstProperties); 3. Give each FunctionLiteral an embedded AstProperties. 4. Give the visitor a single embedded AstProperties. 5. Simply copy from (factory's) visitor to function literal when constructing a function literal? https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2356 src/ast.h:2356: zone_(zone), Since every call (that I could find) does not pass zone or visitor, I'd just leave them off the parameter list and assume their default values (I don't understand what is the semantics of passing a zone other than isolate->zone() anyway). https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2370 src/ast.h:2370: visitor_->Visit##NodeType(node); \ Possibly: visitor_->Visit##NodeType((node)); to allow a comma expression as 'node'. https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2399 src/ast.h:2399: ITERATION_STATEMENT(SwitchStatement) Switch is not iteration, so this is probably a bad name for the macro. https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2460 src/ast.h:2460: static EmptyStatement* empty = ::new EmptyStatement(); It seems wrong to have this singleton empty statement. The AST is no longer a tree, the nodes no longer have a unique identity, and I'm not sure it's really any optimization. I'd take this opportunity to get rid of it. https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2617 src/ast.h:2617: bool visit_with_visitor = true) { No need for default value. Boolean default values are extra scary. I also don't like two boolean parameters in a row. https://chromiumcodereview.appspot.com/9221011/diff/13003/src/compiler.h File src/compiler.h (right): https://chromiumcodereview.appspot.com/9221011/diff/13003/src/compiler.h#newcode1 src/compiler.h:1: // Copyright 2012 the V8 project authors. All rights reserved. On 2012/02/06 14:14:29, fschneider wrote:
No changes in this file.
I think you mean "no other changes in this file". https://chromiumcodereview.appspot.com/9221011/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
