Bug 140855 - [ES6] Implement ES6 arrow function syntax
Summary: [ES6] Implement ES6 arrow function syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://wiki.ecmascript.org/doku.php?i...
Keywords:
Depends on: 148034 151983 153862 144954 144955 144956 145108 145132 145638 146394 146507 146934 147742 148055 148148 148445 149338 149410 149615 149855 150893 152028 152497 152537 152570 152575 153864 153977 153981 154027 154517 154639 155153 155491 156059
Blocks: 80559 143889
  Show dependency treegraph
 
Reported: 2015-01-24 04:26 PST by GSkachkov
Modified: 2016-12-12 17:03 PST (History)
16 users (show)

See Also:


Attachments
Patch (126.33 KB, patch)
2015-02-27 13:37 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (69.50 KB, patch)
2015-02-27 18:23 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (69.50 KB, patch)
2015-02-28 00:55 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (69.88 KB, patch)
2015-02-28 02:49 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (73.00 KB, patch)
2015-02-28 04:10 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (752.04 KB, application/zip)
2015-02-28 05:05 PST, Build Bot
no flags Details
Patch (78.90 KB, patch)
2015-03-04 13:12 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (78.89 KB, patch)
2015-03-04 14:00 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (835.70 KB, application/zip)
2015-03-04 16:01 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (1.02 MB, application/zip)
2015-03-04 16:24 PST, Build Bot
no flags Details
Patch (78.58 KB, patch)
2015-03-04 23:44 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (79.85 KB, patch)
2015-03-10 15:04 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (81.15 KB, patch)
2015-03-11 05:46 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (122.29 KB, patch)
2015-03-26 01:56 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (138.29 KB, patch)
2015-03-26 15:59 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (138.48 KB, patch)
2015-03-27 06:28 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (140.14 KB, patch)
2015-03-27 07:14 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (140.09 KB, patch)
2015-03-27 11:04 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (118.77 KB, patch)
2015-03-28 10:19 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (129.62 KB, patch)
2015-03-31 15:37 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.30 KB, patch)
2015-04-01 13:53 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.07 KB, patch)
2015-04-01 14:38 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.19 KB, patch)
2015-04-01 15:12 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.32 KB, patch)
2015-04-02 02:23 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.37 KB, patch)
2015-04-02 02:39 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.41 KB, patch)
2015-04-02 03:02 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (130.34 KB, patch)
2015-04-02 04:11 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (128.12 KB, patch)
2015-04-09 00:07 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (128.99 KB, patch)
2015-04-09 01:15 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (128.90 KB, patch)
2015-04-09 02:21 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (128.84 KB, patch)
2015-04-09 05:46 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (129.62 KB, patch)
2015-04-09 06:27 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2015-04-12 14:09 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (28.41 KB, patch)
2015-04-13 00:07 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2015-04-13 00:22 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (133.68 KB, patch)
2015-04-13 01:01 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2015-04-13 04:09 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (134.93 KB, patch)
2015-04-13 09:47 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (134.95 KB, patch)
2015-04-13 09:57 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (133.37 KB, patch)
2015-04-13 10:18 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (911 bytes, patch)
2015-04-13 10:26 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (133.44 KB, patch)
2015-04-13 10:42 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (14.43 KB, patch)
2015-04-16 04:54 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (15.02 KB, patch)
2015-04-16 05:02 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (14.87 KB, patch)
2015-04-17 11:03 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (143.41 KB, patch)
2015-04-17 12:17 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (142.00 KB, patch)
2015-04-17 14:47 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (142.01 KB, patch)
2015-04-17 15:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (141.55 KB, patch)
2015-04-17 15:13 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (143.14 KB, patch)
2015-04-17 15:20 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch.1(Init) (4.58 KB, patch)
2015-04-17 15:50 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (141.55 KB, patch)
2015-04-17 15:55 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (143.14 KB, patch)
2015-04-17 16:03 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (144.94 KB, patch)
2015-04-19 14:25 PDT, GSkachkov
fpizlo: review-
Details | Formatted Diff | Diff
Patch (31.39 KB, patch)
2015-05-01 06:06 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (59.09 KB, patch)
2015-05-01 12:19 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (77.66 KB, patch)
2015-05-01 15:17 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (208.14 KB, patch)
2015-05-01 15:22 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (208.09 KB, patch)
2015-05-01 16:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (203.29 KB, patch)
2015-05-01 16:18 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (203.29 KB, patch)
2015-05-01 16:26 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (201.69 KB, patch)
2015-05-01 16:55 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (201.69 KB, patch)
2015-05-01 17:14 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (201.00 KB, patch)
2015-05-01 17:30 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (202.57 KB, patch)
2015-05-01 17:37 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (202.86 KB, patch)
2015-05-05 15:21 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (202.84 KB, patch)
2015-05-05 15:31 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch.2(Main) (204.77 KB, patch)
2015-05-05 16:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch.3(Fix review Comments) (20.98 KB, patch)
2015-05-07 12:18 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (186.05 KB, patch)
2015-05-07 15:04 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Full patch. Used for test build (186.22 KB, patch)
2015-05-07 15:38 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-01-24 04:26:21 PST
Description of the syntax of the Arrow Function. 
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-definitions
Comment 1 GSkachkov 2015-02-27 13:37:34 PST
Created attachment 247543 [details]
Patch
Comment 2 GSkachkov 2015-02-27 18:23:25 PST
Created attachment 247579 [details]
Patch
Comment 3 GSkachkov 2015-02-28 00:55:46 PST
Created attachment 247598 [details]
Patch
Comment 4 GSkachkov 2015-02-28 02:49:06 PST
Created attachment 247599 [details]
Patch
Comment 5 GSkachkov 2015-02-28 04:10:49 PST
Created attachment 247600 [details]
Patch
Comment 6 WebKit Commit Bot 2015-02-28 04:12:38 PST
Attachment 247600 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 GSkachkov 2015-02-28 04:17:29 PST
Will not do the fixing this 3 style check errors, possible was chosen some wrong approach. Will wait until review result.
Comment 8 Build Bot 2015-02-28 05:05:07 PST
Comment on attachment 247600 [details]
Patch

Attachment 247600 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5546162463440896

New failing tests:
fast/css/object-fit/object-fit-canvas.html
Comment 9 Build Bot 2015-02-28 05:05:13 PST
Created attachment 247601 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 10 GSkachkov 2015-02-28 05:20:16 PST
(In reply to comment #9)
> Created attachment 247601 [details]
> Archive of layout-test-results from ews105 for mac-mavericks-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

That strange. My patch doesn't touch the failed tests.
Comment 11 GSkachkov 2015-02-28 09:13:02 PST
(In reply to comment #5)
> Created attachment 247600 [details]
> Patch

Supported cases:
 () => 2*2
 x => x*x
 (x, y) => x*y
Comment 12 GSkachkov 2015-03-04 13:12:57 PST
Created attachment 247886 [details]
Patch
Comment 13 WebKit Commit Bot 2015-03-04 13:17:02 PST
Attachment 247886 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 GSkachkov 2015-03-04 14:00:28 PST
Created attachment 247894 [details]
Patch
Comment 15 WebKit Commit Bot 2015-03-04 14:01:52 PST
Attachment 247894 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Build Bot 2015-03-04 16:01:54 PST
Comment on attachment 247894 [details]
Patch

Attachment 247894 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5010282615144448

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T2.html
http/tests/workers/worker-importScripts.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T9.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T7.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.3_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T3.html
js/dom/parse-error-external-script-in-eval.html
js/kde/garbage-n.html
js/dom/parse-error-external-script-in-new-Function.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.4_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T8.html
sputnik/Conformance/12_Statement/12.11_The_switch_Statement/S12.11_A3_T5.html
Comment 17 Build Bot 2015-03-04 16:01:59 PST
Created attachment 247906 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 18 Build Bot 2015-03-04 16:24:07 PST
Comment on attachment 247894 [details]
Patch

Attachment 247894 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5960117048573952

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T2.html
http/tests/workers/worker-importScripts.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T9.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T7.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.3_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T3.html
js/dom/parse-error-external-script-in-eval.html
js/kde/garbage-n.html
js/dom/parse-error-external-script-in-new-Function.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.4_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T8.html
sputnik/Conformance/12_Statement/12.11_The_switch_Statement/S12.11_A3_T5.html
Comment 19 Build Bot 2015-03-04 16:24:11 PST
Created attachment 247909 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 20 GSkachkov 2015-03-04 23:44:01 PST
Created attachment 247934 [details]
Patch
Comment 21 WebKit Commit Bot 2015-03-04 23:45:37 PST
Attachment 247934 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Yusuke Suzuki 2015-03-06 08:14:34 PST
Good start!
Do you have a plan to implement |this| binding semantics of arrow functions?
Are you planning to implement it in the subsequent patches? :D
Comment 23 GSkachkov 2015-03-06 11:25:33 PST
(In reply to comment #22)
> Good start!
> Do you have a plan to implement |this| binding semantics of arrow functions?
> Are you planning to implement it in the subsequent patches? :D

Thanks for comment. 

I'm goint to implement full spec of the arrow function. But for now I'm tring to follow the webkit contribution rule - deliver the smollest path/feature. Also I'm quite new in webkit project and expect that I will redo a lot in current patch to reach webkit standards in quality, performance and etc :-)
Comment 24 Yusuke Suzuki 2015-03-06 12:31:19 PST
One tip: we can write tests related to JavaScriptCore under 'tests/stress' instead of layout-tests. fpizlo told me this very nice test infrastructure :)
Comment 25 Yusuke Suzuki 2015-03-06 12:34:00 PST
(In reply to comment #24)
> One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> instead of layout-tests. fpizlo told me this very nice test infrastructure :)

And execute it with `Tools/Scripts/run-javascriptcore-tests` :D
Comment 26 GSkachkov 2015-03-09 12:15:19 PDT
(In reply to comment #24)
> One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> instead of layout-tests. fpizlo told me this very nice test infrastructure :)

Thanks for the tip. I'll do this.
Yusuke Suzuki, What do you think, do I need add/change something inside of the tests, for instance cover some special cases or cover some special exception and etc?
Comment 27 GSkachkov 2015-03-10 15:04:10 PDT
Created attachment 248363 [details]
Patch
Comment 28 Yusuke Suzuki 2015-03-10 15:14:59 PDT
(In reply to comment #26)
> (In reply to comment #24)
> > One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> > instead of layout-tests. fpizlo told me this very nice test infrastructure :)
> 
> Thanks for the tip. I'll do this.
> Yusuke Suzuki, What do you think, do I need add/change something inside of
> the tests, for instance cover some special cases or cover some special
> exception and etc?

Ideally, I think there's 3 edge cases for arrow functions in the ES6 spec.
If I missed / misunderstood something, please point it :)

1. Arrow Function's environment doesn't has |this| binding.

So, |this| should be lookuped from outer side.
And Function.prototype.bind for ArrowFunction doesn't work well according to the spec.
And since `class syntax` is now landed (like https://trac.webkit.org/changeset/181293), be careful for treating |this| and TDZ. (at some point, |this| is not accessable)

2. Arrow Function doesn't has [[Construct]]

So, arrow function with `new` call should be raise TypeError.

3. Don't create "arguments" for arrow function

arrow function doesn't have "arguments" in its environment.
Comment 29 Yusuke Suzuki 2015-03-10 15:18:44 PDT
(In reply to comment #26)
> Thanks for the tip. I'll do this.
> Yusuke Suzuki, What do you think, do I need add/change something inside of
> the tests, for instance cover some special cases or cover some special
> exception and etc?

And I think tests for syntax errors are needed.
Let's write syntax error tests with `eval` and `SyntaxError` :D
Comment 30 GSkachkov 2015-03-11 05:46:55 PDT
Created attachment 248414 [details]
Patch
Comment 31 WebKit Commit Bot 2015-03-11 05:50:12 PDT
Attachment 248414 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:716:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:717:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 GSkachkov 2015-03-11 05:54:50 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> > > instead of layout-tests. fpizlo told me this very nice test infrastructure :)
> > 
> > Thanks for the tip. I'll do this.
> > Yusuke Suzuki, What do you think, do I need add/change something inside of
> > the tests, for instance cover some special cases or cover some special
> > exception and etc?
> 
> Ideally, I think there's 3 edge cases for arrow functions in the ES6 spec.
> If I missed / misunderstood something, please point it :)
> 
> 1. Arrow Function's environment doesn't has |this| binding.
> 
> So, |this| should be lookuped from outer side.
> And Function.prototype.bind for ArrowFunction doesn't work well according to
> the spec.
> And since `class syntax` is now landed (like
> https://trac.webkit.org/changeset/181293), be careful for treating |this|
> and TDZ. (at some point, |this| is not accessable)
> 
> 2. Arrow Function doesn't has [[Construct]]
> 
> So, arrow function with `new` call should be raise TypeError.
> 
> 3. Don't create "arguments" for arrow function
> 
> arrow function doesn't have "arguments" in its environment.

Thanks for feedback. 
I'll try to cover this 3 edge cases soon.
Comment 33 Saam Barati 2015-03-15 23:41:50 PDT
Comment on attachment 248414 [details]
Patch

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

Patch looks good overall. I think you can touch it up a bit before landing.

> Source/JavaScriptCore/ChangeLog:16
> +

It's worth going through some of this to check for spelling/grammatical errors. There are many 
of these such errors.

Also, it's nice to have some annotations of the functions you changed below,
but stay away from restating what the changelog already states. Your comments
should give insight as to why you changed something or how something works.
It's not worth saying something like: "Added extra parameter". The diff below
will tell us this.

> Source/JavaScriptCore/parser/ASTBuilder.h:-314
> -    

You should revert this whitespace and all other white space changes below.

> Source/JavaScriptCore/parser/Parser.cpp:584
> +    semanticFailIfFalse(currentScope()->isFunction(), "Return statements are only valid inside arrow functions");

I'm confused by this error message here. Is it actually supposed to say that?

> Source/JavaScriptCore/parser/Parser.cpp:1502
> +template <class TreeBuilder> bool Parser<LexerType>::parseArrowFunctionInfo(TreeBuilder& context, FunctionParseMode mode, ConstructorKind constructorKind, ParserFunctionInfo<TreeBuilder>& info)

This function looks like it has a ton of duplicated code with parseFunctionInfo(.).
It's worth creating an abstraction that handles both function info's so we don't
end up with two divergent code paths in the future.

> Source/JavaScriptCore/parser/Parser.cpp:1524
> +            info.parameters = parseFormalParameters(context);

I don't think this will do what you want. This will parse:
x,y => {}
I think the syntax only allows for single parameter arrow functions to not have parens around
their formal parameter list.
You will probably want to failIfFalse if there isn't only a single parameter here

> Source/JavaScriptCore/parser/Parser.cpp:1642
> +        bool isEndOfArrowFunction = match(SEMICOLON) || match(COMMA) || match(CLOSEPAREN) || match(CLOSEBRACE) || match(CLOSEBRACKET);

Nit: It may be worth writing a function to do this since
you have almost this same expression above.

> Source/JavaScriptCore/parser/Parser.h:84
> +enum FunctionParseType { StandardFunctionParseType, ArrowFunctionParseType };

I think we should use a better name for this because parseSourceElements(.) is used to parse more than functions.
Maybe something as simple as SourceElementsParseType

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

Is there another bug open for the correct implementation of the arrow function to have
lexical "this" scoping?

I think it's probably okay to land this in two different bugs, but you'll probably end up changing some 
of the code in this patch.

We should also have a lot of parsing tests for this patch on top of these tests.
For example, I think your patch as it stands will incorrectly parse:
x,y => x + y;
Comment 34 GSkachkov 2015-03-17 03:37:29 PDT
(In reply to comment #33)
> Comment on attachment 248414 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248414&action=review
> 
> Patch looks good overall. I think you can touch it up a bit before landing.
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +
> 
> It's worth going through some of this to check for spelling/grammatical
> errors. There are many 
> of these such errors.
> 
> Also, it's nice to have some annotations of the functions you changed below,
> but stay away from restating what the changelog already states. Your comments
> should give insight as to why you changed something or how something works.
> It's not worth saying something like: "Added extra parameter". The diff below
> will tell us this.
> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:-314
> > -    
> 
> You should revert this whitespace and all other white space changes below.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:584
> > +    semanticFailIfFalse(currentScope()->isFunction(), "Return statements are only valid inside arrow functions");
> 
> I'm confused by this error message here. Is it actually supposed to say that?
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1502
> > +template <class TreeBuilder> bool Parser<LexerType>::parseArrowFunctionInfo(TreeBuilder& context, FunctionParseMode mode, ConstructorKind constructorKind, ParserFunctionInfo<TreeBuilder>& info)
> 
> This function looks like it has a ton of duplicated code with
> parseFunctionInfo(.).
> It's worth creating an abstraction that handles both function info's so we
> don't
> end up with two divergent code paths in the future.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1524
> > +            info.parameters = parseFormalParameters(context);
> 
> I don't think this will do what you want. This will parse:
> x,y => {}
> I think the syntax only allows for single parameter arrow functions to not
> have parens around
> their formal parameter list.
> You will probably want to failIfFalse if there isn't only a single parameter
> here
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1642
> > +        bool isEndOfArrowFunction = match(SEMICOLON) || match(COMMA) || match(CLOSEPAREN) || match(CLOSEBRACE) || match(CLOSEBRACKET);
> 
> Nit: It may be worth writing a function to do this since
> you have almost this same expression above.
> 
> > Source/JavaScriptCore/parser/Parser.h:84
> > +enum FunctionParseType { StandardFunctionParseType, ArrowFunctionParseType };
> 
> I think we should use a better name for this because parseSourceElements(.)
> is used to parse more than functions.
> Maybe something as simple as SourceElementsParseType
> 
> > Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> > +var result;
> 
> Is there another bug open for the correct implementation of the arrow
> function to have
> lexical "this" scoping?
> 
> I think it's probably okay to land this in two different bugs, but you'll
> probably end up changing some 
> of the code in this patch.
> 
> We should also have a lot of parsing tests for this patch on top of these
> tests.
> For example, I think your patch as it stands will incorrectly parse:
> x,y => x + y;

Thank you for this deep review. 
I'm finishing fixing comments from Yusuke Suzuki, after that I'll switch to the fixing of your comments.
Comment 35 GSkachkov 2015-03-26 01:56:46 PDT
Created attachment 249479 [details]
Patch
Comment 36 GSkachkov 2015-03-26 15:59:07 PDT
Created attachment 249529 [details]
Patch
Comment 37 WebKit Commit Bot 2015-03-26 16:02:52 PDT
Attachment 249529 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 GSkachkov 2015-03-26 16:06:33 PDT
(In reply to comment #36)
> Created attachment 249529 [details]
> Patch

Ohhh, It was deep & long dive into core of the JSC.
I've implemented 1&2 second comments from Yusuke Suzuki.
Now I'm working on adding the description of all changes into Changlog and increasing the numbers of test cases. Hope after that can start work on comments from Saam Barati.
Comment 39 GSkachkov 2015-03-27 06:28:27 PDT
Created attachment 249563 [details]
Patch
Comment 40 WebKit Commit Bot 2015-03-27 06:30:50 PDT
Attachment 249563 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 GSkachkov 2015-03-27 07:14:22 PDT
Created attachment 249565 [details]
Patch
Comment 42 WebKit Commit Bot 2015-03-27 07:17:42 PDT
Attachment 249565 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 GSkachkov 2015-03-27 11:04:00 PDT
Created attachment 249587 [details]
Patch
Comment 44 WebKit Commit Bot 2015-03-27 11:08:10 PDT
Attachment 249587 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 GSkachkov 2015-03-28 10:19:56 PDT
Created attachment 249664 [details]
Patch
Comment 46 WebKit Commit Bot 2015-03-28 10:22:12 PDT
Attachment 249664 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 47 Saam Barati 2015-03-28 12:15:03 PDT
Comment on attachment 249664 [details]
Patch

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

Looking pretty good. Some comments below:

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> +    RegisterID* value = emitThisNodeBytecode(r0, n->position());

I don't think this is doing exactly what you want here.
r0 here is the destination of this function expression, and you will 
move lexical 'this' into this destination, then, you will override that
with the function expression. I don't think this is the intention. I'm
not exactly sure of the consequences of this are, but it may be easier to
get rid of the ::emitThisNodeBytecode function, and just do this here:
```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
or, you may even just be able to do:
```instructions().append(thisRegister()->index())```

> Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);

I don't think you want to load 'this' after the above branch because
you need it at the branch destination. (same for 32-bit version below).

> Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);

Nit: I would call this 'executable' or 'function' or something other than 'funcExpr'.

> Source/JavaScriptCore/jit/JITOperations.cpp:951
> +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));

Maybe we should give this a name here instead of ""?
We should investigate this.

Also, is 0 the correct thing to pass here?
I'm not really sure what that does.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));

ditto.

Also, maybe it's worth having some kind of helper function here 
because this function is so similar to the above.

> Source/JavaScriptCore/parser/Parser.cpp:1538
> +{

I still think this function has too much code in common with ::parseFunctionInfo,
we should abstract away the common bits.

> Source/JavaScriptCore/parser/Parser.cpp:2080
> +#endif

Nit: I think that can just be one contiguous region, not need for #endif then #if

> Source/JavaScriptCore/parser/Parser.h:560
> +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)

Nit: whitespace

> Source/JavaScriptCore/parser/Parser.h:568
> +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,

ditto

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

I think we need some tests for lexical 'this'.
Comment 48 GSkachkov 2015-03-31 15:37:13 PDT
Created attachment 249862 [details]
Patch
Comment 49 WebKit Commit Bot 2015-03-31 15:40:54 PDT
Attachment 249862 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 GSkachkov 2015-03-31 15:48:39 PDT
(In reply to comment #47)
> Comment on attachment 249664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249664&action=review
> 
> Looking pretty good. Some comments below:
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> > +    RegisterID* value = emitThisNodeBytecode(r0, n->position());
> 
> I don't think this is doing exactly what you want here.
> r0 here is the destination of this function expression, and you will 
> move lexical 'this' into this destination, then, you will override that
> with the function expression. I don't think this is the intention. I'm
> not exactly sure of the consequences of this are, but it may be easier to
> get rid of the ::emitThisNodeBytecode function, and just do this here:
> ```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
> or, you may even just be able to do:
> ```instructions().append(thisRegister()->index())```
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> > +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);
> 
> I don't think you want to load 'this' after the above branch because
> you need it at the branch destination. (same for 32-bit version below).
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> > +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
> 
> Nit: I would call this 'executable' or 'function' or something other than
> 'funcExpr'.
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:951
> > +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));
> 
> Maybe we should give this a name here instead of ""?
> We should investigate this.
> 
> Also, is 0 the correct thing to pass here?
> I'm not really sure what that does.
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> > +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));
> 
> ditto.
> 
> Also, maybe it's worth having some kind of helper function here 
> because this function is so similar to the above.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1538
> > +{
> 
> I still think this function has too much code in common with
> ::parseFunctionInfo,
> we should abstract away the common bits.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2080
> > +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif
> then #if
> 
> > Source/JavaScriptCore/parser/Parser.h:560
> > +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)
> 
> Nit: whitespace
> 
> > Source/JavaScriptCore/parser/Parser.h:568
> > +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,
> 
> ditto
> 
> > Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> > +var result;
> 
> I think we need some tests for lexical 'this'.

I really appreciate your feedback. It allow to fix several failing tests. 
I've tried to fix most of the comments with new patch. I'll provide more detail for each your comments after checking the build by CI.
Comment 51 GSkachkov 2015-04-01 13:53:08 PDT
Created attachment 249940 [details]
Patch
Comment 52 WebKit Commit Bot 2015-04-01 13:56:41 PDT
Attachment 249940 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 GSkachkov 2015-04-01 14:12:07 PDT
Comment on attachment 249664 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:985
>> +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);
> 
> I don't think you want to load 'this' after the above branch because
> you need it at the branch destination. (same for 32-bit version below).

I've changed position of the loading 'this'. I'm hope it fixed

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:995
>> +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
> 
> Nit: I would call this 'executable' or 'function' or something other than 'funcExpr'.

done

>> Source/JavaScriptCore/jit/JITOperations.cpp:951
>> +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));
> 
> Maybe we should give this a name here instead of ""?
> We should investigate this.
> 
> Also, is 0 the correct thing to pass here?
> I'm not really sure what that does.

Not sure that we should add the name of the function. I've checked this parameters with on following code 
var f = function (x) { return x + 1}.bind(this); and name had value "" and number of additional parameters  were 0.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
>> +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));
> 
> ditto.
> 
> Also, maybe it's worth having some kind of helper function here 
> because this function is so similar to the above.

I've tried to eliminate of code duplication by using DEFINE. Is it correct to use this approach in such cases?

>> Source/JavaScriptCore/parser/Parser.cpp:1538
>> +{
> 
> I still think this function has too much code in common with ::parseFunctionInfo,
> we should abstract away the common bits.

I've merged parseArrowFunctionInfo with parseFunctionInfo and parseArrowFunctioBody with  parseFunctionBody. Hope they are still readable.

>> Source/JavaScriptCore/parser/Parser.cpp:2080
>> +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif then #if

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
>> +var result;
> 
> I think we need some tests for lexical 'this'.

I've added one more Source/JavaScriptCore/tests/stress/arrowfunction-lexicalthis.js file to  test lexical 'this'.
Comment 54 GSkachkov 2015-04-01 14:38:59 PDT
Created attachment 249942 [details]
Patch
Comment 55 WebKit Commit Bot 2015-04-01 14:41:49 PDT
Attachment 249942 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 GSkachkov 2015-04-01 15:12:33 PDT
Created attachment 249946 [details]
Patch
Comment 57 WebKit Commit Bot 2015-04-01 15:15:36 PDT
Attachment 249946 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 GSkachkov 2015-04-02 02:23:47 PDT
Created attachment 249965 [details]
Patch
Comment 59 WebKit Commit Bot 2015-04-02 02:26:49 PDT
Attachment 249965 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 GSkachkov 2015-04-02 02:39:52 PDT
Created attachment 249966 [details]
Patch
Comment 61 WebKit Commit Bot 2015-04-02 02:42:18 PDT
Attachment 249966 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 GSkachkov 2015-04-02 03:02:10 PDT
Created attachment 249967 [details]
Patch
Comment 63 WebKit Commit Bot 2015-04-02 03:03:46 PDT
Attachment 249967 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 64 GSkachkov 2015-04-02 04:11:26 PDT
Created attachment 249968 [details]
Patch
Comment 65 WebKit Commit Bot 2015-04-02 04:14:16 PDT
Attachment 249968 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Ryosuke Niwa 2015-04-02 10:24:26 PDT
Comment on attachment 249968 [details]
Patch

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

I ran out of the time to review mid way but this should be a plenty of feedback to work on :)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
> +    instructions().append(thisRegister()->index());

Are we emitting a TDZ check if an arrow function is used inside a derived class' constructor before super() is called?
e.g. new (class extends (class {}) { () => this; super(); }) should throw a ReferenceError instead of crashing.

Please add a stress test too.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)

How is this code different at all from what's in ThisNode::emitBytecode?
I don't think we should duplicate this much code in two places.

> Source/JavaScriptCore/jit/JITOperations.cpp:47
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +#include "JSArrowFunction.h"
> +#endif

We should probably be checking this inside JSArrowFunction.h

> Source/JavaScriptCore/jit/JITOperations.h:121
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EJscCJ)(ExecState*, JSScope*, JSCell*, EncodedJSValue);
> +#endif

I don't think we need to if-def op codes like this.  e.g. op_check_tdz is used only when class syntax is enabled but we don't if-def it.

> Source/JavaScriptCore/parser/Parser.cpp:598
> +    bool correcArrowFinish = isEndOfArrowFunction() || match(EOFTOK);

Typo: correc*t*.  How is this condition different from autoSemiColon()?
It seems that we need to document the difference one way or another.

> Source/JavaScriptCore/parser/Parser.cpp:1198
> +    bool handled = false;

This variable is unnecessary.

> Source/JavaScriptCore/parser/Parser.cpp:1206
> +            handled = true;

We should just call context.setEndOffset and return here instead.

> Source/JavaScriptCore/parser/Parser.cpp:1457
> +    if (parseType == ArrowFunctionParseType) {

We should either use switch here or assert that parseType == ArrowFunctionParseType if the previous if evaluated to false.
Also, can we do a refactoring to put the original code into a separate helper function in a separate patch so that the diff here is readable?
As is, it's hard to review...

> Source/JavaScriptCore/parser/Parser.cpp:1523
> +        if (parseType == ArrowFunctionParseType) {
> +            info.arrowFunctionBodyType = cachedInfo->isBodyArrowFunctionBlock ? ArrowFunctionBodyBlock : ArrowFunctionBodyExpression;

Why do we need to condition this based on parseType?
Since arrowFunctionBodyType is never used, it should be fine to always set info.arrowFunctionBodyType.

> Source/JavaScriptCore/parser/Parser.cpp:1528
> +            m_token = info.arrowFunctionBodyType == ArrowFunctionBodyBlock ? cachedInfo->closeBraceToken() : cachedInfo->endArrowFunctionToken();
> +        } else
> +            m_token = cachedInfo->closeBraceToken();
> +#else   
>          m_token = cachedInfo->closeBraceToken();

Can't we just add endFunctionToken to cacheInfo instead?

> Source/JavaScriptCore/parser/Parser.cpp:1544
> +        if (parseType == ArrowFunctionParseType) {
> +            if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock)
> +                next();
> +        } else
> +            next();

!?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

> Source/JavaScriptCore/parser/Parser.cpp:1629
> +    if (parseType == ArrowFunctionParseType) {
> +        if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock) {
> +            matchOrFail(CLOSEBRACE, "Expected a closing '}' after a ", stringForFunctionMode(mode), " body");
> +            next();
> +        } else

It looks like these two conditions can be combined into parseType == ArrowFunctionParseType && info.arrowFunctionBodyType != ArrowFunctionBodyBlock
to run the code in else since the statement in the inner if and the outer else are identical.
Comment 67 GSkachkov 2015-04-03 07:20:35 PDT
Ryosuke Niwa  Thanks for the review!
I'll try to fix soon.
Comment 68 GSkachkov 2015-04-09 00:07:02 PDT
Created attachment 250421 [details]
Patch
Comment 69 WebKit Commit Bot 2015-04-09 00:08:38 PDT
Attachment 250421 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 70 GSkachkov 2015-04-09 00:45:19 PDT
Comment on attachment 249968 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
>> +    instructions().append(thisRegister()->index());
> 
> Are we emitting a TDZ check if an arrow function is used inside a derived class' constructor before super() is called?
> e.g. new (class extends (class {}) { () => this; super(); }) should throw a ReferenceError instead of crashing.
> 
> Please add a stress test too.

I've added test Source/JavaScriptCore/tests/stress/arrowfunction-tdz.js.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
>> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)
> 
> How is this code different at all from what's in ThisNode::emitBytecode?
> I don't think we should duplicate this much code in two places.

Removed duplication.

>> Source/JavaScriptCore/jit/JITOperations.cpp:47
>> +#endif
> 
> We should probably be checking this inside JSArrowFunction.h

Done.

>> Source/JavaScriptCore/jit/JITOperations.h:121
>> +#endif
> 
> I don't think we need to if-def op codes like this.  e.g. op_check_tdz is used only when class syntax is enabled but we don't if-def it.

removed if-def

>> Source/JavaScriptCore/parser/Parser.cpp:598
>> +    bool correcArrowFinish = isEndOfArrowFunction() || match(EOFTOK);
> 
> Typo: correc*t*.  How is this condition different from autoSemiColon()?
> It seems that we need to document the difference one way or another.

I'll take a look deeper in autoSemiColon(), possible this functions both do the same.

>> Source/JavaScriptCore/parser/Parser.cpp:1198
>> +    bool handled = false;
> 
> This variable is unnecessary.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1206
>> +            handled = true;
> 
> We should just call context.setEndOffset and return here instead.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1457
>> +    if (parseType == ArrowFunctionParseType) {
> 
> We should either use switch here or assert that parseType == ArrowFunctionParseType if the previous if evaluated to false.
> Also, can we do a refactoring to put the original code into a separate helper function in a separate patch so that the diff here is readable?
> As is, it's hard to review...

I'll try to refactor to increase patch readability this weekend.

>> Source/JavaScriptCore/parser/Parser.cpp:1528
>>          m_token = cachedInfo->closeBraceToken();
> 
> Can't we just add endFunctionToken to cacheInfo instead?

Introduced new function with name endFunctionToken instead of closeBraceToken() & endArrowFunctionToken

>> Source/JavaScriptCore/parser/Parser.cpp:1544
>> +            next();
> 
> !?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

Fixed

>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> +        } else
> 
> It looks like these two conditions can be combined into parseType == ArrowFunctionParseType && info.arrowFunctionBodyType != ArrowFunctionBodyBlock
> to run the code in else since the statement in the inner if and the outer else are identical.

Done
Comment 71 GSkachkov 2015-04-09 01:15:34 PDT
Created attachment 250425 [details]
Patch
Comment 72 WebKit Commit Bot 2015-04-09 01:17:46 PDT
Attachment 250425 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 73 GSkachkov 2015-04-09 02:21:01 PDT
Created attachment 250430 [details]
Patch
Comment 74 WebKit Commit Bot 2015-04-09 02:24:04 PDT
Attachment 250430 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 75 GSkachkov 2015-04-09 05:46:23 PDT
Created attachment 250437 [details]
Patch
Comment 76 WebKit Commit Bot 2015-04-09 05:48:16 PDT
Attachment 250437 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 77 GSkachkov 2015-04-09 06:27:30 PDT
Created attachment 250438 [details]
Patch
Comment 78 WebKit Commit Bot 2015-04-09 06:30:16 PDT
Attachment 250438 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 79 GSkachkov 2015-04-12 14:09:14 PDT
Created attachment 250618 [details]
Patch
Comment 80 GSkachkov 2015-04-13 00:07:52 PDT
Created attachment 250626 [details]
Patch
Comment 81 GSkachkov 2015-04-13 00:22:37 PDT
Created attachment 250628 [details]
Patch
Comment 82 GSkachkov 2015-04-13 01:01:51 PDT
Created attachment 250629 [details]
Patch
Comment 83 GSkachkov 2015-04-13 04:09:04 PDT
Created attachment 250633 [details]
Patch
Comment 84 GSkachkov 2015-04-13 09:47:49 PDT
Created attachment 250643 [details]
Patch
Comment 85 GSkachkov 2015-04-13 09:57:45 PDT
Created attachment 250647 [details]
Patch
Comment 86 WebKit Commit Bot 2015-04-13 10:01:41 PDT
Attachment 250647 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 87 GSkachkov 2015-04-13 10:18:01 PDT
Created attachment 250649 [details]
Patch
Comment 88 GSkachkov 2015-04-13 10:26:46 PDT
Created attachment 250651 [details]
Patch
Comment 89 GSkachkov 2015-04-13 10:42:02 PDT
Created attachment 250656 [details]
Patch
Comment 90 GSkachkov 2015-04-16 04:54:09 PDT
Created attachment 250911 [details]
Patch
Comment 91 GSkachkov 2015-04-16 05:02:28 PDT
Created attachment 250912 [details]
Patch
Comment 92 Darin Adler 2015-04-17 09:15:33 PDT
Comment on attachment 250912 [details]
Patch

Please rebase the patch again so it applies and the EWS can run the tests.
Comment 93 GSkachkov 2015-04-17 11:03:39 PDT
Created attachment 251029 [details]
Patch
Comment 94 GSkachkov 2015-04-17 12:17:32 PDT
Created attachment 251037 [details]
Patch
Comment 95 WebKit Commit Bot 2015-04-17 12:19:32 PDT
Attachment 251037 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 96 GSkachkov 2015-04-17 14:47:07 PDT
Created attachment 251050 [details]
Patch
Comment 97 GSkachkov 2015-04-17 15:00:21 PDT
Created attachment 251055 [details]
Patch
Comment 98 GSkachkov 2015-04-17 15:13:47 PDT
Created attachment 251056 [details]
Patch
Comment 99 GSkachkov 2015-04-17 15:20:05 PDT
Created attachment 251057 [details]
Patch
Comment 100 WebKit Commit Bot 2015-04-17 15:22:28 PDT
Attachment 251057 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 101 GSkachkov 2015-04-17 15:50:45 PDT
Created attachment 251059 [details]
Patch.1(Init)
Comment 102 GSkachkov 2015-04-17 15:55:17 PDT
Created attachment 251061 [details]
Patch
Comment 103 GSkachkov 2015-04-17 16:03:50 PDT
Created attachment 251063 [details]
Patch
Comment 104 WebKit Commit Bot 2015-04-17 16:07:53 PDT
Attachment 251063 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 105 GSkachkov 2015-04-18 00:24:24 PDT
(In reply to comment #92)
> Comment on attachment 250912 [details]
> Patch
> 
> Please rebase the patch again so it applies and the EWS can run the tests.

Done!
Comment 106 GSkachkov 2015-04-19 14:25:59 PDT
Created attachment 251129 [details]
Patch
Comment 107 WebKit Commit Bot 2015-04-19 14:29:53 PDT
Attachment 251129 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 108 GSkachkov 2015-04-19 14:42:04 PDT
(In reply to comment #105)
> (In reply to comment #92)
> > Comment on attachment 250912 [details]
> > Patch
> > 
> > Please rebase the patch again so it applies and the EWS can run the tests.
> 
> Done!

I've made small refactoring and added new test. It would be great to have new review :)
Comment 109 Filip Pizlo 2015-04-19 15:05:45 PDT
Comment on attachment 251129 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3755
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +        case op_new_arrow_func_exp: {
> +            FunctionExecutable* expr = m_inlineStackTop->m_profiledBlock->functionExpr(currentInstruction[3].u.operand);
> +            FrozenValue* frozen = m_graph.freezeStrong(expr);
> +            set(VirtualRegister(currentInstruction[1].u.operand),
> +                addToGraph(NewFunction, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
> +            NEXT_OPCODE(op_new_arrow_func_exp);
> +        }
> +#endif

This looks like it has to be wrong since you're not using operand 4.
Comment 110 GSkachkov 2015-05-01 06:06:13 PDT
Created attachment 252149 [details]
Patch
Comment 111 GSkachkov 2015-05-01 12:19:53 PDT
Created attachment 252164 [details]
Patch
Comment 112 GSkachkov 2015-05-01 15:17:57 PDT
Created attachment 252174 [details]
Patch
Comment 113 GSkachkov 2015-05-01 15:22:31 PDT
Created attachment 252175 [details]
Patch
Comment 114 GSkachkov 2015-05-01 16:00:54 PDT
Created attachment 252184 [details]
Patch
Comment 115 GSkachkov 2015-05-01 16:18:02 PDT
Created attachment 252189 [details]
Patch
Comment 116 GSkachkov 2015-05-01 16:26:11 PDT
Created attachment 252191 [details]
Patch
Comment 117 GSkachkov 2015-05-01 16:55:21 PDT
Created attachment 252199 [details]
Patch
Comment 118 GSkachkov 2015-05-01 17:14:52 PDT
Created attachment 252201 [details]
Patch
Comment 119 GSkachkov 2015-05-01 17:30:08 PDT
Created attachment 252204 [details]
Patch
Comment 120 GSkachkov 2015-05-01 17:37:19 PDT
Created attachment 252205 [details]
Patch
Comment 121 WebKit Commit Bot 2015-05-01 17:41:42 PDT
Attachment 252205 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:802:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 122 GSkachkov 2015-05-01 23:28:13 PDT
Comment on attachment 252205 [details]
Patch

It strange that gtk-wk2/elf-wk2 build were failed. I didn't do anything gtk-wk2/elf-wk2 related in my source code
Comment 123 GSkachkov 2015-05-03 16:13:57 PDT
(In reply to comment #109)
> Comment on attachment 251129 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251129&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3755
> > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> > +        case op_new_arrow_func_exp: {
> > +            FunctionExecutable* expr = m_inlineStackTop->m_profiledBlock->functionExpr(currentInstruction[3].u.operand);
> > +            FrozenValue* frozen = m_graph.freezeStrong(expr);
> > +            set(VirtualRegister(currentInstruction[1].u.operand),
> > +                addToGraph(NewFunction, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
> > +            NEXT_OPCODE(op_new_arrow_func_exp);
> > +        }
> > +#endif
> 
> This looks like it has to be wrong since you're not using operand 4.

Thanks for review. Comment is fixed in new patch.
Comment 124 GSkachkov 2015-05-05 15:21:07 PDT
Created attachment 252411 [details]
Patch
Comment 125 WebKit Commit Bot 2015-05-05 15:24:23 PDT
Attachment 252411 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 126 GSkachkov 2015-05-05 15:31:52 PDT
Created attachment 252412 [details]
Patch
Comment 127 WebKit Commit Bot 2015-05-05 15:36:24 PDT
Attachment 252412 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 128 GSkachkov 2015-05-05 16:00:44 PDT
Created attachment 252416 [details]
Patch.2(Main)
Comment 129 WebKit Commit Bot 2015-05-05 16:04:53 PDT
Attachment 252416 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 130 Filip Pizlo 2015-05-05 16:23:07 PDT
Comment on attachment 252416 [details]
Patch.2(Main)

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1541
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Why do you need this guard?  I recommend adding the arraowFunctionStructure() to JSGlobalObject unconditionally.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:234
> +    case op_new_arrow_func_exp: // Fixme: Arrow function cann't be inline as the common function because it has lexicaly bind this/arguments and etc. and  arrow function has to have its own compile and inline approach.

I don't buy this comment at all.  Sounds like either there is a horrible bug, or you could just allow it.

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:134
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Ditto to my previous.  Get rid of this guard.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2992
> +    void compileNewArrowFunction()

If you're going to implement it in the FTL, then also implement it in DFG.
Comment 131 GSkachkov 2015-05-07 12:18:20 PDT
Created attachment 252606 [details]
Patch.3(Fix review Comments)
Comment 132 GSkachkov 2015-05-07 13:03:27 PDT
Comment on attachment 252416 [details]
Patch.2(Main)

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

I've added separate patch with fixes.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1541
>> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> 
> Why do you need this guard?  I recommend adding the arraowFunctionStructure() to JSGlobalObject unconditionally.

Done

>> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:234
>> +    case op_new_arrow_func_exp: // Fixme: Arrow function cann't be inline as the common function because it has lexicaly bind this/arguments and etc. and  arrow function has to have its own compile and inline approach.
> 
> I don't buy this comment at all.  Sounds like either there is a horrible bug, or you could just allow it.

Implemented.

>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:134
>> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> 
> Ditto to my previous.  Get rid of this guard.

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2992
>> +    void compileNewArrowFunction()
> 
> If you're going to implement it in the FTL, then also implement it in DFG.

I've implemented it in DFG.
Comment 133 GSkachkov 2015-05-07 15:04:33 PDT
Created attachment 252633 [details]
Patch
Comment 134 WebKit Commit Bot 2015-05-07 15:09:03 PDT
Attachment 252633 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 135 GSkachkov 2015-05-07 15:38:14 PDT
Created attachment 252637 [details]
Full patch. Used for test build
Comment 136 WebKit Commit Bot 2015-05-07 15:43:45 PDT
Attachment 252637 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 137 GSkachkov 2015-05-13 12:26:02 PDT
Comment on attachment 251059 [details]
Patch.1(Init)

Patch moved in subtask. https://bugs.webkit.org/show_bug.cgi?id=144954
Comment 138 Csaba Osztrogonác 2015-09-14 11:13:57 PDT
Comment on attachment 251059 [details]
Patch.1(Init)

Cleared Filip Pizlo's review+ from obsolete attachment 251059 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 139 Joseph Pecoraro 2016-12-12 11:05:18 PST
Can this bug be closed now? We've had support for Arrow functions for a while. Or are the remaining bugs this depends on still relevant?
Comment 140 Saam Barati 2016-12-12 17:03:49 PST
I think this should be closed. All things left are optimizations and miscellaneous things, which aren't blocking the actual feature.