Bug 139416

Summary: Removed some allocation and cruft from the parser
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, mark.lam, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 139428    
Attachments:
Description Flags
Patch mark.lam: review+

Description Geoffrey Garen 2014-12-08 14:55:33 PST
Removed some allocation and cruft from the parser
Comment 1 Geoffrey Garen 2014-12-08 15:51:57 PST
Created attachment 242857 [details]
Patch
Comment 2 Mark Lam 2014-12-08 16:51:17 PST
Comment on attachment 242857 [details]
Patch

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

r=me with one suggestion.

> Source/JavaScriptCore/parser/SyntaxChecker.h:133
> +    ExpressionType appendToCommaExpr(const JSTokenLocation&, ExpressionType& tail, ExpressionType next) { tail = next; return next; }

I suggest changing this prototype to match the one in ASTBuilder (tail can be just ExpressionType).  Also, you no longer need to assign "tail = next;" here since you've changed the parser to do the assign using the return value.  Hence, this "tail" assignment is redundant.
Comment 3 Geoffrey Garen 2014-12-08 17:13:25 PST
> > Source/JavaScriptCore/parser/SyntaxChecker.h:133
> > +    ExpressionType appendToCommaExpr(const JSTokenLocation&, ExpressionType& tail, ExpressionType next) { tail = next; return next; }
> 
> I suggest changing this prototype to match the one in ASTBuilder (tail can
> be just ExpressionType).  Also, you no longer need to assign "tail = next;"
> here since you've changed the parser to do the assign using the return
> value.  Hence, this "tail" assignment is redundant.

You're right that I wrote this redundantly with its caller; I'll fix that. But the code will continue to need a distinct signature. This code is a bit evil. The ASTBuilder builds a valid linked list of comma nodes. But the SyntaxChecker actually builds an invalid list that constantly overwrites its head with its tail. The reason it does this is so that the parent expression of the comma list can statically check for errors caused by the value of the last node (which is the node that produces the comma expression's value). By overwriting the head with the tail, the syntax checker presents only the tail node of the comma list to its parent expression for checking.
Comment 4 Geoffrey Garen 2014-12-08 17:14:27 PST
For example, the program

    'use strict'; ++(1, 2, 3, eval)

is a syntax error.
Comment 5 Mark Lam 2014-12-08 17:31:24 PST
(In reply to comment #3)
> By overwriting the head with the tail, the syntax checker presents only the
> tail node of the comma list to its parent expression for checking.

Interesting.  So tricky.
Comment 6 Geoffrey Garen 2014-12-08 17:53:16 PST
Committed r177001: <http://trac.webkit.org/changeset/177001>
Comment 7 Gyuyoung Kim 2014-12-08 21:24:02 PST
EFL port has broken since r177001. Because EFL port treats build warning as error. This patch generated a below warning.


../../Source/JavaScriptCore/parser/Nodes.h:1104:11: error: direct base ‘JSC::ParserArenaFreeable’ inaccessible in ‘JSC::CommaNode’ due to ambiguity [-Werror]
     class CommaNode final : public ExpressionNode, public ParserArenaFreeable {
           ^
Comment 8 Gyuyoung Kim 2014-12-08 21:43:29 PST
(In reply to comment #7)
> EFL port has broken since r177001. Because EFL port treats build warning as
> error. This patch generated a below warning.
> 
> 
> ../../Source/JavaScriptCore/parser/Nodes.h:1104:11: error: direct base
> ‘JSC::ParserArenaFreeable’ inaccessible in ‘JSC::CommaNode’ due to ambiguity
> [-Werror]
>      class CommaNode final : public ExpressionNode, public
> ParserArenaFreeable {
>            ^

I upload a fix to Bug 139428.