"a minimal change"? Yeah, right ;-)
But I agree that the CL looks better now. All comments addressed, please
take
another look.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.cc
File src/ast.cc (right):
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.cc#newcode1020
src/ast.cc:1020: // proxy()->var()->IsStackAllocated() && fun() == NULL;
On 2012/02/06 14:14:29, fschneider wrote:
Forgotten commented code?
No, two-line comment :-)
It was intended as a reminder for a possible future optimization: we
could do an early exit (before re-parsing) from
HGraphBuilder::TryInline() if the persisted AstProperties information
told us that some declaration is not inlineable anyway.
But that's probably not going to happen now anyway, so I've just removed
the comment.
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#newcode1492
src/ast.h:1492: ? static_cast<int>(GetNextId(isolate))
On 2012/02/06 14:14:29, fschneider wrote:
GetNextId returns unsigned, yet all ast ids are stored as int. Does it
just work
changing GetNextId to return an int instead?
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2322
src/ast.h:2322: AstProperties* DetachAstProperties();
On 2012/02/06 15:14:23, Kevin Millikin wrote:
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?
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2329
src/ast.h:2329: virtual void Visit##type(type* node);
On 2012/02/06 14:14:29, fschneider wrote:
It seems that none of the functions actually recursively visit their
children.
In this case you can avoid inheriting from AstVisitor and make all the
functions
non-virtual.
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2340
src/ast.h:2340: // hold the results; is passed to VariableProxies upon
their construction
On 2012/02/06 14:14:29, fschneider wrote:
Comment outdated? VariableProxies don't update their info later
anymore, do
they?
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2356
src/ast.h:2356: zone_(zone),
On 2012/02/06 14:14:29, fschneider wrote:
Maybe simplify initialization to:
zone_(zone != NULL ? zone : isolate->zone()),
Obsolete -- see Kevin's comment.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2356
src/ast.h:2356: zone_(zone),
On 2012/02/06 15:14:23, Kevin Millikin wrote:
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).
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2370
src/ast.h:2370: visitor_->Visit##NodeType(node); \
On 2012/02/06 14:14:29, fschneider wrote:
Suggestion for further optimization:
If the visitor was embedded in the AstNodeFactory (or even
AstNodeFactory
includes all from AstConstructionVisitor), you could get rid of the
additional
indirection and avoid checking visitor_ == NULL for each New* call.
You could create a templated version of AstNodeFactory<bool
is_visiting> where
the template parameters controls if you do the visitor-action or not.
I think it
would be a minimal change to the code overall.
The FunctionState then would have an instance of AstNodeFactory
embedded instead
of having an AstConstructionVisitor and save/restore while parsing it
in the
FunctionState constructor/destructor.
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2370
src/ast.h:2370: visitor_->Visit##NodeType(node); \
On 2012/02/06 15:14:23, Kevin Millikin wrote:
Possibly:
visitor_->Visit##NodeType((node));
to allow a comma expression as 'node'.
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2399
src/ast.h:2399: ITERATION_STATEMENT(SwitchStatement)
On 2012/02/06 15:14:23, Kevin Millikin wrote:
Switch is not iteration, so this is probably a bad name for the macro.
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2460
src/ast.h:2460: static EmptyStatement* empty = ::new EmptyStatement();
On 2012/02/06 15:14:23, Kevin Millikin wrote:
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.
Done.
https://chromiumcodereview.appspot.com/9221011/diff/13003/src/ast.h#newcode2617
src/ast.h:2617: bool visit_with_visitor = true) {
On 2012/02/06 15:14:23, Kevin Millikin wrote:
No need for default value. Boolean default values are extra scary.
Done.
I also don't like two boolean parameters in a row.
Done.
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.
Done.
https://chromiumcodereview.appspot.com/9221011/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
