Bug 139416 – Removed some allocation and cruft from the parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Removed some allocation and cruft from the parser

[Geoffrey Garen]

Created attachment 242857 [details]
Patch

[Mark Lam]

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.

[Geoffrey Garen]

> > 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.

[Geoffrey Garen]

For example, the program

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

is a syntax error.

[Mark Lam]

(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.

[Geoffrey Garen]

Committed r177001: <http://trac.webkit.org/changeset/177001>

[Gyuyoung Kim]

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 {
           ^

[Gyuyoung Kim]

(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.