Bug 135545

Summary: The parser should generate AST nodes the var declarations with no initializers
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Severity: Normal CC: commit-queue, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 141070    
Description Flags
work in progress
ggaren: review-
patch none

Description Saam Barati 2014-08-02 15:49:30 PDT
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.
Comment 1 Saam Barati 2014-08-02 16:09:54 PDT
Created attachment 235940 [details]
work in progress

What do you guys think of this patch?
Comment 2 Geoffrey Garen 2014-08-04 15:20:04 PDT
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.
Comment 3 Saam Barati 2014-08-15 16:43:14 PDT
Created attachment 236691 [details]

Patch with Geoff's suggestions.
Comment 4 Geoffrey Garen 2014-08-18 11:39:25 PDT
Comment on attachment 236691 [details]

Comment 5 WebKit Commit Bot 2014-08-18 12:11:55 PDT
Comment on attachment 236691 [details]

Clearing flags on attachment: 236691

Committed r172717: <http://trac.webkit.org/changeset/172717>
Comment 6 WebKit Commit Bot 2014-08-18 12:11:58 PDT
All reviewed patches have been landed.  Closing bug.