| Summary: | assignments in for-in/for-of header not allowed | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Saam Barati
2016-03-11 15:14:07 PST
Created attachment 273785 [details]
patch
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. Thanks for the review. I made the suggestions you recommended. landed in: http://trac.webkit.org/changeset/198144 |