Bug 135545 – The parser should generate AST nodes the var declarations with no initializers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Currently, when the parser is parsing a variable declaration list and sees a var declarator without an initializer, it just adds the variable name to the environment in the parsing context, but does not actually create an AST node when there is no initializer. 
Because it doesn't create an AST node, the helper function that calls this parsing method may emit an EmptyExpression AST node, or may emit a CommaNode where there aren't nodes in the CommaNode representing empty initializers.
We should emit an AST node here that represents declaring variables without initializers because it gives high fidelity type profiling the opportunity to emit code that observes this variable.

[Saam Barati]

Created attachment 235940 [details]
work in progress

What do you guys think of this patch?

[Geoffrey Garen]

Comment on attachment 235940 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=235940&action=review

I think this approach is good. Some edits are in order.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1789
> +    generator.emitDebugHook(WillExecuteStatement, lineNo(), startOffset(), lineStartOffset());

This seems like it might be too much debug hook. Now, we'll have two debug hooks for the statement "var x;": one for the VarStatementNode, and one for the EmptyVarExpressionNode. Probably the right thing to do is not to debug hook here, since usually only statements debug hook, and we don't stop the debugger in the middle of expressions.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1791
> +    if (generator.isProfilingTypesWithHighFidelity()) {

It's nice to make this a "not" check followed by early return. That way, the rest of the code doesn't have to indent, and you rarely get super-indented code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1796
> +            //RegisterID* finalDest = generator.finalDestination(dst);

Nix this.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1804
> +    // It's safe to return null here because this node will always be part of VarStatementNode which ignores our return value.

"part of" => "a child node of"

> Source/JavaScriptCore/parser/ASTBuilder.h:649
> -    
> +

Revert whitespace change please.

[Saam Barati]

Created attachment 236691 [details]
patch

Patch with Geoff's suggestions.

[Geoffrey Garen]

Comment on attachment 236691 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 236691 [details]
patch

Clearing flags on attachment: 236691

Committed r172717: <http://trac.webkit.org/changeset/172717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.