| Summary: | [ES6] Implement ES6 arrow function syntax | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | GSkachkov <gskachkov> |
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | barraclough, basile_clement, bburg, benjamin, bjonesbe, buildbot, commit-queue, dbates, fpizlo, ggaren, joepeck, mark.lam, rniwa, saam, ysuzuki, zmyaro |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| URL: | http://wiki.ecmascript.org/doku.php?id=harmony:arrow_function_syntax | ||
| Bug 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 | ||
| Bug Blocks: | 80559, 143889 | ||
| Attachments: | |||
|
Description
GSkachkov
2015-01-24 04:26:21 PST
Created attachment 247543 [details]
Patch
Created attachment 247579 [details]
Patch
Created attachment 247598 [details]
Patch
Created attachment 247599 [details]
Patch
Created attachment 247600 [details]
Patch
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.
Will not do the fixing this 3 style check errors, possible was chosen some wrong approach. Will wait until review result. 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 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
(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. (In reply to comment #5) > Created attachment 247600 [details] > Patch Supported cases: () => 2*2 x => x*x (x, y) => x*y Created attachment 247886 [details]
Patch
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.
Created attachment 247894 [details]
Patch
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 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 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 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 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
Created attachment 247934 [details]
Patch
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.
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 (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 :-) One tip: we can write tests related to JavaScriptCore under 'tests/stress' instead of layout-tests. fpizlo told me this very nice test infrastructure :) (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 (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? Created attachment 248363 [details]
Patch
(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. (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 Created attachment 248414 [details]
Patch
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.
(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 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; (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. Created attachment 249479 [details]
Patch
Created attachment 249529 [details]
Patch
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.
(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. Created attachment 249563 [details]
Patch
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.
Created attachment 249565 [details]
Patch
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.
Created attachment 249587 [details]
Patch
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.
Created attachment 249664 [details]
Patch
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 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'. Created attachment 249862 [details]
Patch
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.
(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. Created attachment 249940 [details]
Patch
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 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'. Created attachment 249942 [details]
Patch
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.
Created attachment 249946 [details]
Patch
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.
Created attachment 249965 [details]
Patch
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.
Created attachment 249966 [details]
Patch
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.
Created attachment 249967 [details]
Patch
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.
Created attachment 249968 [details]
Patch
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 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. Ryosuke Niwa Thanks for the review! I'll try to fix soon. Created attachment 250421 [details]
Patch
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 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 Created attachment 250425 [details]
Patch
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.
Created attachment 250430 [details]
Patch
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.
Created attachment 250437 [details]
Patch
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.
Created attachment 250438 [details]
Patch
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.
Created attachment 250618 [details]
Patch
Created attachment 250626 [details]
Patch
Created attachment 250628 [details]
Patch
Created attachment 250629 [details]
Patch
Created attachment 250633 [details]
Patch
Created attachment 250643 [details]
Patch
Created attachment 250647 [details]
Patch
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.
Created attachment 250649 [details]
Patch
Created attachment 250651 [details]
Patch
Created attachment 250656 [details]
Patch
Created attachment 250911 [details]
Patch
Created attachment 250912 [details]
Patch
Comment on attachment 250912 [details]
Patch
Please rebase the patch again so it applies and the EWS can run the tests.
Created attachment 251029 [details]
Patch
Created attachment 251037 [details]
Patch
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.
Created attachment 251050 [details]
Patch
Created attachment 251055 [details]
Patch
Created attachment 251056 [details]
Patch
Created attachment 251057 [details]
Patch
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.
Created attachment 251059 [details]
Patch.1(Init)
Created attachment 251061 [details]
Patch
Created attachment 251063 [details]
Patch
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.
(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! Created attachment 251129 [details]
Patch
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.
(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 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. Created attachment 252149 [details]
Patch
Created attachment 252164 [details]
Patch
Created attachment 252174 [details]
Patch
Created attachment 252175 [details]
Patch
Created attachment 252184 [details]
Patch
Created attachment 252189 [details]
Patch
Created attachment 252191 [details]
Patch
Created attachment 252199 [details]
Patch
Created attachment 252201 [details]
Patch
Created attachment 252204 [details]
Patch
Created attachment 252205 [details]
Patch
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 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
(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. Created attachment 252411 [details]
Patch
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.
Created attachment 252412 [details]
Patch
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.
Created attachment 252416 [details]
Patch.2(Main)
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 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. Created attachment 252606 [details]
Patch.3(Fix review Comments)
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. Created attachment 252633 [details]
Patch
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.
Created attachment 252637 [details]
Full patch. Used for test build
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 on attachment 251059 [details] Patch.1(Init) Patch moved in subtask. https://bugs.webkit.org/show_bug.cgi?id=144954 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. 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? I think this should be closed. All things left are optimizations and miscellaneous things, which aren't blocking the actual feature. |