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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 16585: Optimize variable declarations
http://bugs.webkit.org/show_bug.cgi?id=16585

Attachment 18071: optimize var statements - 3.7% speedup
http://bugs.webkit.org/attachment.cgi?id=18071&action=edit

------- 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
ExprStatementNode<betterNameThanIsVar>::

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

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.