[webkit-reviews] review granted: [Bug 16585] Optimize variable declarations : [Attachment 18071] optimize var statements - 3.7% speedup

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 23 15:58:46 PST 2007

Eric Seidel <eric at webkit.org> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 16585: Optimize variable declarations

Attachment 18071: optimize var statements - 3.7% speedup

------- Additional Comments from Eric Seidel <eric at webkit.org>
So my comments deal entirely with code readability.  AFAICT, the functionality
looks fine:

ExprStatementNode<false> and ExprStatementNode<true> should have nicely named
typdefs.  Probably ExprStatementNode should also change names to some sort of
ExprStatementNodeBase or the like.

As part of that <isVar> needs a more descriptive name.	Small functions would
be better done using the typdef classes, larger functions can be defined using

If you look at the formatted diff, it appears you added a whole bunch of extra

I think passing int attrs as part of appendToVarDeclarationList and using
DeclarationStacks::IsConstant and DeclarationStacks::HasInitializerat the
callsites would be clearer.  At least at the callsites. :)

 1807	    ForNode(ExpressionNode* e1, ExpressionNode* e2, ExpressionNode* e3,
StatementNode* s, bool e1WasVarDecl) KJS_FAST_CALL

I was trying to come up with a cleaner way of expressing the same.  Using an
enum "FirstExprIsVarDecl" instead of a bool would lead to cleaner callsites...
I guess that's about the best I can come up with for right now.

If you fix at least some of those readability issues, this is totally fine for
landing.  Congrats on such an awesome speedup.

More information about the webkit-reviews mailing list