[webkit-reviews] review denied: [Bug 135545] [ftlopt] The parser should emit AST nodes the var declarations with no initializers : [Attachment 235940] work in progress

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 135545: [ftlopt] The parser should emit AST nodes the var declarations with
no initializers
https://bugs.webkit.org/show_bug.cgi?id=135545

Attachment 235940: work in progress
https://bugs.webkit.org/attachment.cgi?id=235940&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.