Bug 155384 - assignments in for-in/for-of header not allowed
Summary: assignments in for-in/for-of header not allowed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-11 15:14 PST by Saam Barati
Modified: 2016-03-14 11:38 PDT (History)
10 users (show)

See Also:

Attachments
patch (13.43 KB, patch)
2016-03-11 16:49 PST, Saam Barati 		darin: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-03-11 15:14:07 PST 
...
Comment 1 Saam Barati 2016-03-11 16:49:33 PST 
Created attachment 273785 [details]
patch
Comment 2 Darin Adler 2016-03-13 14:41:39 PDT 
Comment on attachment 273785 [details]
patch

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

> Source/JavaScriptCore/parser/ASTBuilder.h:185
> +    bool isNewTarget(ExpressionNode* expr) { return expr->isNewTarget(); }

I suggest either the word "expression" or the word "node" rather than the abbreviation "expr".

> Source/JavaScriptCore/parser/Nodes.h:562
> +        bool isNewTarget() const override { return true; }

We’ve been choosing to “final” rather than “override” in cases like this one in our virtual function style discussions for the WebKit project. Also, should just make it private since there’s no point calling it if you already have a reference or pointer to a NewTargetNode.
Comment 3 Saam Barati 2016-03-14 11:38:55 PDT 
Thanks for the review. I made the suggestions you recommended.
landed in:
http://trac.webkit.org/changeset/198144