Bug 144955

Created attachment 253039 [details] Patch

Attachment 253039 [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 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 253069 [details] Patch

Attachment 253069 [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 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 253195 [details] Patch

Attachment 253195 [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 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 253195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253195&action=review > Source/JavaScriptCore/parser/ASTBuilder.h:372 > + ? m_sourceCode->subArrowExpression(info.arrowFunctionOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn) > + : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn); Wrong indentation. ? and : should be exactly 4 spaces to the right from the beginning of "SourceCode source". > Source/JavaScriptCore/parser/ASTBuilder.h:375 > + info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset); Ditto. Indent the second line by exactly 4 spaces further to the right (12 total spaces). > Source/JavaScriptCore/parser/Lexer.cpp:1824 > + tokenData->line = savedlineNumber; > + tokenData->offset = savedOffset; > + tokenData->lineStartOffset = savedLineStartOffset; > + > + ASSERT(tokenData->offset >= tokenData->lineStartOffset); I don't think we want to shift() and then set offset back manually like this. We should use peek() instead. > Source/JavaScriptCore/parser/Parser.cpp:605 > + setEndOfStatement(); Why do we need this? > Source/JavaScriptCore/parser/Parser.cpp:1203 > + context.setEndOffset(result, m_lastTokenEndPosition.offset); What is this setEndOffset for? > Source/JavaScriptCore/parser/Parser.cpp:1260 > + // Consume the end of arrow function ,;]) and prevent > + // parse this symbols by parseStatement as separate statement I don't understand this comment. In general, we prefer adding assertions to document conditions rather than adding comments. Is there a way to express this as an assertion instead? > Source/JavaScriptCore/parser/Parser.cpp:1341 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > + if (parseType == StandardFunctionParseType) { > + next(); > + if (match(CLOSEBRACE)) { > + unsigned endColumn = tokenColumn(); > + return context.createFunctionBody(startLocation, tokenLocation(), startColumn, endColumn, functionKeywordStart, functionNameStart, parametersStart, strictMode(), constructorKind); > + } > + } > +#else It's not great to duplicate the code like this. Since FunctionParseType is always define, we should just use that without wrapping in if-def.

(In reply to comment #7) > Comment on attachment 253195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253195&action=review > > > Source/JavaScriptCore/parser/ASTBuilder.h:372 > > + ? m_sourceCode->subArrowExpression(info.arrowFunctionOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn) > > + : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn); > > Wrong indentation. ? and : should be exactly 4 spaces to the right from the > beginning of "SourceCode source". > > > Source/JavaScriptCore/parser/ASTBuilder.h:375 > > + info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset); > > Ditto. Indent the second line by exactly 4 spaces further to the right (12 > total spaces). > > > Source/JavaScriptCore/parser/Lexer.cpp:1824 > > + tokenData->line = savedlineNumber; > > + tokenData->offset = savedOffset; > > + tokenData->lineStartOffset = savedLineStartOffset; > > + > > + ASSERT(tokenData->offset >= tokenData->lineStartOffset); > > I don't think we want to shift() and then set offset back manually like this. > We should use peek() instead. > > > Source/JavaScriptCore/parser/Parser.cpp:605 > > + setEndOfStatement(); > > Why do we need this? > > > Source/JavaScriptCore/parser/Parser.cpp:1203 > > + context.setEndOffset(result, m_lastTokenEndPosition.offset); > > What is this setEndOffset for? I believe this is for the controlFlowProfiler > > > Source/JavaScriptCore/parser/Parser.cpp:1260 > > + // Consume the end of arrow function ,;]) and prevent > > + // parse this symbols by parseStatement as separate statement > > I don't understand this comment. > In general, we prefer adding assertions to document conditions rather than > adding comments. > Is there a way to express this as an assertion instead? > > > Source/JavaScriptCore/parser/Parser.cpp:1341 > > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > + if (parseType == StandardFunctionParseType) { > > + next(); > > + if (match(CLOSEBRACE)) { > > + unsigned endColumn = tokenColumn(); > > + return context.createFunctionBody(startLocation, tokenLocation(), startColumn, endColumn, functionKeywordStart, functionNameStart, parametersStart, strictMode(), constructorKind); > > + } > > + } > > +#else > > It's not great to duplicate the code like this. > Since FunctionParseType is always define, we should just use that without > wrapping in if-def.

Comment on attachment 253195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253195&action=review This looks pretty good to me. It may be worth opening up some follow up bugs that will be aimed at getting this to be spec compliant (i.e lexical this, etc) and adding this bug as blocking those bugs. It will make it explicit what this patch does and doesn't have implemented. >> Source/JavaScriptCore/parser/Parser.cpp:1203 >> + context.setEndOffset(result, m_lastTokenEndPosition.offset); > > What is this setEndOffset for? Actually, this is wrong. How does this code not segfault? setEndOffset is meant to be called with non-null AST nodes > Source/JavaScriptCore/parser/Parser.cpp:1427 > + consume(OPENPAREN); Nit: you can use next() here. We already know it's an open paren > Source/JavaScriptCore/parser/Parser.cpp:2852 > + while (newCount--) This should be tested. > Source/JavaScriptCore/parser/Parser.cpp:2854 > + return base; Do we currently throw when trying to 'new' an arrow function?

Created attachment 253296 [details] Patch

I've extracted all changes that related to the adding of the arrow function feature flag to the project in another task https://bugs.webkit.org/show_bug.cgi?id=145108, current task is depends on 145108, and can be landed only after 145108.

Comment on attachment 253195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253195&action=review >>> Source/JavaScriptCore/parser/ASTBuilder.h:372 >>> + : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn); >> >> Wrong indentation. ? and : should be exactly 4 spaces to the right from the beginning of "SourceCode source". > > I believe this is for the controlFlowProfiler Fixed. Please check if correct. >> Source/JavaScriptCore/parser/ASTBuilder.h:375 >> + info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset); > > Ditto. Indent the second line by exactly 4 spaces further to the right (12 total spaces). Why do we need indent for 'info.body->setLoc()' statement? >> Source/JavaScriptCore/parser/Lexer.cpp:1824 >> + ASSERT(tokenData->offset >= tokenData->lineStartOffset); > > I don't think we want to shift() and then set offset back manually like this. > We should use peek() instead. Done >> Source/JavaScriptCore/parser/Parser.cpp:605 >> + setEndOfStatement(); > > Why do we need this? Ohh, it will be difficult to explain with my English, but I'll try. I need invoke this function to set correctly position inside of the lexer. It is added by me in Source/JavaScriptCore/parser/Lexer.cpp line 1723 and repeat the same statement as do for '}' in lexer https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1990. It is necessary because incase of arrow function with expression (var f = x=> x+1;) symbols ';', ']', '}', ')', and ',' has two meaning first it is end of statement and second end of arrow function body. I haven't found way move this logic to lexer because only parser knowing context when it is only end of statement or end of arrow function body as well. >>> Source/JavaScriptCore/parser/Parser.cpp:1203 >>> + context.setEndOffset(result, m_lastTokenEndPosition.offset); >> >> What is this setEndOffset for? > > Actually, this is wrong. How does this code not segfault? > setEndOffset is meant to be called with non-null AST nodes Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10; For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426 >> Source/JavaScriptCore/parser/Parser.cpp:2852 >> + while (newCount--) > > This should be tested. I've removed this code.

(In reply to comment #9) > Comment on attachment 253195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253195&action=review > > This looks pretty good to me. > It may be worth opening up some follow up bugs that > will be aimed at getting this to be spec compliant (i.e lexical this, etc) > and adding this bug as blocking those bugs. It will make it explicit > what this patch does and doesn't have implemented. > > >> Source/JavaScriptCore/parser/Parser.cpp:1203 > >> + context.setEndOffset(result, m_lastTokenEndPosition.offset); > > > > What is this setEndOffset for? > > Actually, this is wrong. How does this code not segfault? > setEndOffset is meant to be called with non-null AST nodes > > > Source/JavaScriptCore/parser/Parser.cpp:1427 > > + consume(OPENPAREN); > > Nit: you can use next() here. We already know it's an open paren > > > Source/JavaScriptCore/parser/Parser.cpp:2852 > > + while (newCount--) > > This should be tested. > > > Source/JavaScriptCore/parser/Parser.cpp:2854 > > + return base; > > Do we currently throw when trying to 'new' an arrow function? As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, also I've add new one https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later.

Attachment 253296 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Parser.cpp:2836: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/parser/Parser.h:738: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 253369 [details] Patch

Attachment 253369 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Parser.h:738: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 253195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253195&action=review >>>>> Source/JavaScriptCore/parser/Parser.cpp:1203 >>>>> + context.setEndOffset(result, m_lastTokenEndPosition.offset); >>>> >>>> What is this setEndOffset for? >>> >>> Actually, this is wrong. How does this code not segfault? >>> setEndOffset is meant to be called with non-null AST nodes >> >> Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; >> Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10; >> For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426 > > As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, > also I've add new one https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later. Right. But, if you look at the code for setEndOffset, it will deference the AST node you pass it (inside ASTBuilder, not SyntaxChecker). This will be bad because we're passing in null for that AST node. On another note, this code path seems a little bit weird to me. Is there an alternate way to check the end of an arrow function that isn't inside parseStatement? To me, this seems like a weird place for this code to live.

Created attachment 253378 [details] Patch

Attachment 253378 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/parser/Parser.h:738: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #17) > Comment on attachment 253195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253195&action=review > > >>>>> Source/JavaScriptCore/parser/Parser.cpp:1203 > >>>>> + context.setEndOffset(result, m_lastTokenEndPosition.offset); > >>>> > >>>> What is this setEndOffset for? > >>> > >>> Actually, this is wrong. How does this code not segfault? > >>> setEndOffset is meant to be called with non-null AST nodes > >> > >> Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; > >> Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10; > >> For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426 > > > > As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, > > also I've add new one https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later. > > Right. But, if you look at the code for setEndOffset, it will deference the > AST node you pass it (inside ASTBuilder, not SyntaxChecker). > This will be bad because we're passing in null for that AST node. > > On another note, this code path seems a little bit weird to me. Is there an > alternate > way to check the end of an arrow function that isn't inside parseStatement? > To me, this seems like a weird place for this code to live. I've checked it seems that this statement is unnecessary. So I've removed it. Tests are still green.

Comment on attachment 253378 [details] Patch Found broken test. Cancel review until fix.

Comment on attachment 253378 [details] Patch Test are green now.

Comment on attachment 253378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253378&action=review Quick comment. Maybe I need to learn the control flow profiler context. Saam is good reviewer for the control flow profiler part. I'll review it again. great work! > Source/JavaScriptCore/parser/Parser.cpp:1202 > + return result; Is there any reason not using `isEndOfArrowFunction`? > Source/JavaScriptCore/parser/Parser.cpp:1259 > + break; This code makes that `=>{}` can be parsed. But it should be SyntaxError. And when passing `=>{}`, assertion failure occurs. > Source/JavaScriptCore/parser/Parser.cpp:1613 > + failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function"); Here, it doesn't allow EOF. So, it seems that `()=>e<EOF>` becomes SyntaxError. Is there any reason not adding EOF to isEndOfArrowFunction? > Source/JavaScriptCore/parser/Parser.cpp:2832 > + functionKeywordStart, info, ArrowFunctionParseType)), "Cannot parse arrow function expression"); Nits: Line breaking is not needed.

Comment on attachment 253378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253378&action=review Patch looks good to me. Great work. Some suggestions below. > Source/JavaScriptCore/ChangeLog:9 > + In patch were implemented the simplest cases of arrow function declaration: "In patch were implemented" => "This patch implements" > Source/JavaScriptCore/ChangeLog:17 > + 2 raising exception in case of trying to use ânewâ with arrow function Nit: Looks like some weird characters around the "new" here. > Source/JavaScriptCore/parser/ASTBuilder.h:372 > + : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn); Nit: It may be worth renaming ParserFunctionInfo's "openBraceOffset" variable to "bodyStartOffset" or something like that and then this ternary expression becomes unnecessary. > Source/JavaScriptCore/parser/Parser.cpp:1968 > +template <class TreeBuilder> TreeExpression Parser<LexerType>::parseArrowFunctionExpression(TreeBuilder& context) In regards to the controlFlowProfiler, this code is ok. To be sure and prevent regressions, it's worth writing a few simple tests for the controlFlowProfiler. If you look inside JavaScriptCore/tests/controlFlowProfiler/* there is a standardized way of doing this. You can do this in another patch or you can open a bug and assign it to me and I'll write some tests for it. On another note, I don't think this code path is worthy of being its own function. I'd just inline this inside parseArrowFunctionStatement because this looks like it's a simple wrapper around parseAssignmentExpression. > Source/JavaScriptCore/parser/Parser.cpp:2026 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) Nit: Would this whole sequence of checking if something is an arrow function be worthy of its own function? > Source/JavaScriptCore/parser/Parser.cpp:2034 > + if (match(ARROWFUNCTION)) This code seems unnecessarily convoluted. I'd move the below code inside the "if (match(ARROWFUNCTION))" into this branch. If we reach this branch we're guaranteed to execute the code of parseArrowFunctionWrapper in the below branch. That way we don't have to worry about the arrowFunctionEmptyList variable and we don't duplicate code inside the "#if/#else". > Source/JavaScriptCore/parser/SourceCode.h:129 > + ASSERT(provider()->source()[endArrowFunction - 1] == ',' || provider()->source()[endArrowFunction - 1] == ';' || provider()->source()[endArrowFunction - 1] == ')' || provider()->source()[endArrowFunction - 1] == ']' || provider()->source()[endArrowFunction - 1] == '}'); I'd just cache the character in a local variable and remove the Fixme and switch to ASSERT_UNUSED. > Source/JavaScriptCore/tests/stress/arrowfunction-asparamter-1.js:1 > +// Inspired by mozilla tests I think some of these tests don't make sense as "stress tests". Maybe they're better inside LayoutTests where we have some similar tests.

Created attachment 254083 [details] Patch

Created attachment 254087 [details] Patch

Attachment 254087 [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:752: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254098 [details] Patch

Attachment 254098 [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:752: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 254098 [details] Patch Attachment 254098 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4615082206035968 New failing tests: js/arrowfunction-syntax.html js/arrowfunction-asparamter-2.html js/arrowfunction-block-2.html js/arrowfunction-associativity-2.html js/arrowfunction-syntax-endings.html js/arrowfunction-asparamter-1.html js/arrowfunction-block-1.html

Created attachment 254114 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 254098 [details] Patch Attachment 254098 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5047395360440320 New failing tests: js/arrowfunction-syntax.html js/arrowfunction-asparamter-2.html js/arrowfunction-block-2.html js/arrowfunction-associativity-2.html js/arrowfunction-syntax-endings.html js/arrowfunction-asparamter-1.html js/arrowfunction-block-1.html

Created attachment 254118 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5

Created attachment 254166 [details] Patch

Attachment 254166 [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:752: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254174 [details] Patch

Attachment 254174 [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:752: The parameter name "functionParseType" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #37) > 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:752: The parameter name > "functionParseType" adds no information, so it should be removed. > [readability/parameter_name] [5] Please fix these!

(In reply to comment #38) > (In reply to comment #37) > > 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:752: The parameter name > > "functionParseType" adds no information, so it should be removed. > > [readability/parameter_name] [5] > > Please fix these! Not sure that I know how. Could you please suggest how to fix them? 1. Source/JavaScriptCore/parser/ParserTokens.h:79 I've added new value ARROWFUNCTION and followed the same style as all enums in this file http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/ParserTokens.h#L75 2. Source/JavaScriptCore/parser/Parser.h In this file I've added new parameter with default value in exist function. I've used default value to decrease number of changes, so to default value I had to add name of the parameter. template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&, const Identifier*& directive, unsigned* directiveLiteralLength = 0, !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!)

(In reply to comment #39) > 1. Source/JavaScriptCore/parser/ParserTokens.h:79 > I've added new value ARROWFUNCTION and followed the same style as all enums > in this file > http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/ > ParserTokens.h#L75 This one is really strange. I’m not sure why we decided to use all caps for all these tokens. Worth discussing with someone, but for now I suppose you should match the style. > 2. Source/JavaScriptCore/parser/Parser.h > In this file I've added new parameter with default value in exist function. > I've used default value to decrease number of changes, so to default value I > had to add name of the parameter. > > template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&, > const Identifier*& directive, unsigned* directiveLiteralLength = 0, > !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!) You don’t need am argument name to have a default value: [...] unsigned* directiveLiteralLength = nullptr, FunctionParseType = StandardFunctionParseType)

Created attachment 254209 [details] Patch

Attachment 254209 [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] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254239 [details] Patch

Attachment 254239 [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] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #40) > (In reply to comment #39) > > 1. Source/JavaScriptCore/parser/ParserTokens.h:79 > > I've added new value ARROWFUNCTION and followed the same style as all enums > > in this file > > http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/ > > ParserTokens.h#L75 > > This one is really strange. I’m not sure why we decided to use all caps for > all these tokens. Worth discussing with someone, but for now I suppose you > should match the style. > > > 2. Source/JavaScriptCore/parser/Parser.h > > In this file I've added new parameter with default value in exist function. > > I've used default value to decrease number of changes, so to default value I > > had to add name of the parameter. > > > > template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&, > > const Identifier*& directive, unsigned* directiveLiteralLength = 0, > > !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!) > > You don’t need am argument name to have a default value: > > [...] unsigned* directiveLiteralLength = nullptr, FunctionParseType = > StandardFunctionParseType) Thanks for hint! I've loaded new version with fix. Could you please suggest what I need to fix also in this patch.

I don’t think I’m the right reviewer for this new JavaScript engine feature. Geoff, who should review?

Comment on attachment 254239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254239&action=review I can review (partially) this if it's limited to parser's change. In profiler part, I think I'm not a right reviewer. > Source/JavaScriptCore/parser/Parser.cpp:2023 > + bool doParseConditionalExpression = false; I think `shouldParseConditionalExpression` is better. > Source/JavaScriptCore/parser/Parser.cpp:2027 > + doParseConditionalExpression = !isArrowFunctionEmptyParamterList(); In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the above `createSavevPoint` and SavePoint are duplicate. Could you change to the implementation that `isArrowFunctionEmptyParamterList` always restore the state? With that implementation, the above SavePoint is not necessary.

Comment on attachment 254239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254239&action=review >> Source/JavaScriptCore/parser/Parser.cpp:2027 >> + doParseConditionalExpression = !isArrowFunctionEmptyParamterList(); > > In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the above `createSavevPoint` and SavePoint are duplicate. > Could you change to the implementation that `isArrowFunctionEmptyParamterList` always restore the state? > With that implementation, the above SavePoint is not necessary. I agree with Yusuke here, the implementation should always restore the save point. It's weird for a function that tests this to sometimes restore and sometimes not depending on if it found a match. You can have that function always restore the save point and then ASSERT that the assumption is held. Also, the structuring of these two if statements is weird to me, we know for sure one or the other will execute. I'd structure it such that if we match an empty arrow function then we early return and then remove the doParseConditionalExpression. Like so: if (emptyArrow) { ASSERT (...) Blah return ... } TreeExpression lhs = parseConditionalExpression(...) ... > Source/JavaScriptCore/tests/stress/arrowfunction-syntax-error.js:9 > + var errorSyntaxError = false; Should we be doing something with the "statement" parameter? > Source/JavaScriptCore/tests/stress/arrowfunction-syntax-error.js:27 > +testCase(forceRaiseException('debug(=>x+1)'), true, "Error: No exception during parse wrong statement '=>{}'"); Also add a test for this syntax error: "x, y => ..." (And maybe other cases, I'm not super familiar with the grammar on arrow functions.)

Created attachment 254438 [details] Patch

Attachment 254438 [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:638: Missing space before ( in while( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/parser/Parser.h:654: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254445 [details] Patch

Attachment 254445 [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] Total errors found: 1 in 39 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 254239 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254239&action=review > > I can review (partially) this if it's limited to parser's change. In > profiler part, I think I'm not a right reviewer. > > > Source/JavaScriptCore/parser/Parser.cpp:2023 > > + bool doParseConditionalExpression = false; > > I think `shouldParseConditionalExpression` is better. > > > Source/JavaScriptCore/parser/Parser.cpp:2027 > > + doParseConditionalExpression = !isArrowFunctionEmptyParamterList(); > > In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the > above `createSavevPoint` and SavePoint are duplicate. > Could you change to the implementation that > `isArrowFunctionEmptyParamterList` always restore the state? > With that implementation, the above SavePoint is not necessary. I've fixed last comments. What do you think, who can help with profiler part?

(In reply to comment #24) > Comment on attachment 253378 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253378&action=review > > > Source/JavaScriptCore/parser/Parser.cpp:1968 > > +template <class TreeBuilder> TreeExpression Parser<LexerType>::parseArrowFunctionExpression(TreeBuilder& context) > > In regards to the controlFlowProfiler, this code is ok. To be sure and > prevent regressions, it's worth writing a few simple tests for the > controlFlowProfiler. > If you look inside JavaScriptCore/tests/controlFlowProfiler/* there is a > standardized way of doing this. You can do this in another patch or you can > open a bug and assign it to me and I'll write some tests for it. > I've created the issue and upload patch with controlFlowProfiler https://bugs.webkit.org/show_bug.cgi?id=145638. Could you please take a look if it make sense?

Comment on attachment 254445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254445&action=review Great. > Source/JavaScriptCore/parser/Parser.cpp:607 > + bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK); `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct? If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check. > Source/JavaScriptCore/parser/Parser.cpp:608 > + failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement"); it seems refering to EOF and Line terminator is needed. > Source/JavaScriptCore/parser/Parser.cpp:1257 > + result = parseArrowFunctionStatement(context, parseType); This `parseArrowFunctionStatement` name confuses me. This function is used when (e) `=> expr;` part, right? If so, I think clearer name is encouraged. > Source/JavaScriptCore/parser/Parser.cpp:1407 > +template <typename LexerType> template <class TreeBuilder> int Parser<LexerType>::parseArrowFunctionParameters(TreeBuilder& context, FunctionParseMode mode, ParserFunctionInfo<TreeBuilder>& info) Why parseArrowFunctionParameters is needed in addition to `parseFunctionParameters`? Is it possible just using `parseFunctionParameters`? > Source/JavaScriptCore/parser/Parser.cpp:1585 > + position.offset++; Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird. > Source/JavaScriptCore/parser/Parser.cpp:1621 > + failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function"); it seems refering to EOF and Line terminator is needed. > Source/JavaScriptCore/parser/Parser.h:632 > + if (match(EOFTOK)) > + return isArrowFunction; > + Is the above match(EOFTOK) check needed? > Source/JavaScriptCore/parser/SourceCode.h:130 > + UChar symbol = provider()->source()[endArrowFunction - 1]; I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed. > Source/JavaScriptCore/parser/SourceCode.h:131 > + ASSERT_UNUSED(symbol, symbol == 13 || symbol == 10 || symbol == ',' || symbol == ';' || symbol == ')' || symbol == ']' || symbol == '}'); What are 13 and 10? Is it possible to use Lexer<...>::isLineTerminator? So are 0x2028/0x2029 is handled? (They are also line terminators.) > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:73 > token.m_location.endOffset = closeBraceOffset + 1; Is this `+1` OK for non `CLOSEBRACE` token? For example, If the arrow function expression is splitted by previous line terminators, I think (almost) arbitrary tokens can come here. Correct?

Created attachment 254763 [details] Patch

Attachment 254763 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 254445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254445&action=review >> Source/JavaScriptCore/parser/Parser.cpp:607 >> + bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK); > > `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct? > If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check. Done >> Source/JavaScriptCore/parser/Parser.cpp:608 >> + failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement"); > > it seems refering to EOF and Line terminator is needed. Done >> Source/JavaScriptCore/parser/Parser.cpp:1257 >> + result = parseArrowFunctionStatement(context, parseType); > > This `parseArrowFunctionStatement` name confuses me. > This function is used when > > (e) `=> expr;` > > part, right? If so, I think clearer name is encouraged. I've renamed it to parseArrowFunctionExpression. >> Source/JavaScriptCore/parser/Parser.cpp:1407 >> +template <typename LexerType> template <class TreeBuilder> int Parser<LexerType>::parseArrowFunctionParameters(TreeBuilder& context, FunctionParseMode mode, ParserFunctionInfo<TreeBuilder>& info) > > Why parseArrowFunctionParameters is needed in addition to `parseFunctionParameters`? > Is it possible just using `parseFunctionParameters`? Yep, I've refactored, please take a look if it looks better. >> Source/JavaScriptCore/parser/Parser.cpp:1585 >> + position.offset++; > > Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird. It set position to <CR>, because there are no TOCKEN NextLine. So we back to last before new line and add move one position forward >> Source/JavaScriptCore/parser/Parser.cpp:1621 >> + failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function"); > > it seems refering to EOF and Line terminator is needed. Done >> Source/JavaScriptCore/parser/Parser.h:632 >> + > > Is the above match(EOFTOK) check needed? Yes. It solve case when you at EOF and uses save point. If you do restoreSavePoint(saveArrowFunctionPoint) it will back to one step before EOF >> Source/JavaScriptCore/parser/SourceCode.h:130 >> + UChar symbol = provider()->source()[endArrowFunction - 1]; > > I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed. Done >> Source/JavaScriptCore/parser/SourceCode.h:131 >> + ASSERT_UNUSED(symbol, symbol == 13 || symbol == 10 || symbol == ',' || symbol == ';' || symbol == ')' || symbol == ']' || symbol == '}'); > > What are 13 and 10? Is it possible to use Lexer<...>::isLineTerminator? So are 0x2028/0x2029 is handled? (They are also line terminators.) I've added new function isLineTerminator. I'm not sure that it will be good idea to add new dependany to this module. As for me Lexer is from another layer, and SourceCode should not interact with it. What do you think? >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:73 >> token.m_location.endOffset = closeBraceOffset + 1; > > Is this `+1` OK for non `CLOSEBRACE` token? For example, If the arrow function expression is splitted by previous line terminators, I think (almost) arbitrary tokens can come here. Correct? Thanks for hint, I missed that case. I've added new property in parameters to store if line terminator was before token

(In reply to comment #58) > Comment on attachment 254445 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254445&action=review > > >> Source/JavaScriptCore/parser/Parser.cpp:607 > >> + bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK); > > > > `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct? > > If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check. > > Done > > >> Source/JavaScriptCore/parser/Parser.cpp:608 > >> + failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement"); > > > > it seems refering to EOF and Line terminator is needed. > > Done > > >> Source/JavaScriptCore/parser/Parser.cpp:1257 > >> + result = parseArrowFunctionStatement(context, parseType); > > > > This `parseArrowFunctionStatement` name confuses me. > > This function is used when > > > > (e) `=> expr;` > > > > part, right? If so, I think clearer name is encouraged. > > I've renamed it to parseArrowFunctionExpression. Is this only used when parsing the sinle expression in the body syntax? (If not, ignore the below.) If so, I think this is a bad name. To me it sounds like the function is named as if it's parsing the entire arrow function as an expression. I'd be more verbose and go with a name like "parseArrowFunctionSingleExpressionBody" or something else that indicates what the function is doing without any ambiguity.

Comment on attachment 254445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254445&action=review >>> Source/JavaScriptCore/parser/Parser.h:632 >>> + >> >> Is the above match(EOFTOK) check needed? > > Yes. > It solve case when you at EOF and uses save point. If you do restoreSavePoint(saveArrowFunctionPoint) it will back to one step before EOF Make sense.

Comment on attachment 254763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254763&action=review Thanks. > Source/JavaScriptCore/ChangeLog:59 > + (JSC::SourceProviderCacheItem::endArrowFunctionToken): It seems that ChangeLog is not updated. I suggest using `Tools/Scripts/webkit-patch upload --update-changelogs`. > Source/JavaScriptCore/parser/Parser.cpp:-1326 > - } OK, the above sequence is related to the function's name. Splitting out from the `parseFunctionParameters` looks reasonable to me. > Source/JavaScriptCore/parser/Parser.h:833 > +#endif Need to drop it. > Source/JavaScriptCore/parser/SourceCode.h:129 > + } Personally I think maintaining 2 same functions is error prone. In fact, RegExpPrototype.cpp is already using isLineTerminator. So I think using Lexer<UChar>::isLineTerminator not so problematic. Ideally, extracting these character related function into the different header from Lexer and this is nice. > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:74 > token.m_location.endOffset = closeBraceOffset + 1; Hmm, if m_type is not CLOSEBRACE, I think it's not correct because the token's endOffset is not closeBraceOffset + 1. And I think now `closeBraceOffset` name looks misleading because m_type may not be close brace.

Created attachment 255189 [details] Patch

Created attachment 255191 [details] Patch

Attachment 255191 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 254763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254763&action=review >> Source/JavaScriptCore/parser/Parser.h:833 >> +#endif > > Need to drop it. Done >> Source/JavaScriptCore/parser/SourceCode.h:129 >> + } > > Personally I think maintaining 2 same functions is error prone. > In fact, RegExpPrototype.cpp is already using isLineTerminator. So I think using Lexer<UChar>::isLineTerminator not so problematic. > Ideally, extracting these character related function into the different header from Lexer and this is nice. Lexer.h already is linked to SourceCode.h so we would be had circular dependancy. Anyway I've modified logic little bit and get rid of the LineTerminator check. >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:74 >> token.m_location.endOffset = closeBraceOffset + 1; > > Hmm, if m_type is not CLOSEBRACE, I think it's not correct because the token's endOffset is not closeBraceOffset + 1. > And I think now `closeBraceOffset` name looks misleading because m_type may not be close brace. I've added additional parameter to store end of the function

(In reply to comment #61) > Comment on attachment 254763 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254763&action=review > > Thanks. > > > Source/JavaScriptCore/ChangeLog:59 > > + (JSC::SourceProviderCacheItem::endArrowFunctionToken): > > It seems that ChangeLog is not updated. > I suggest using `Tools/Scripts/webkit-patch upload --update-changelogs`. > done

Comment on attachment 255191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255191&action=review Great work. Still a few nits remmain. But logic looks good to me :-) It seems that there are some unitialized fields. Please ensure that all fields are initialized even in StandardFunction type. > Source/JavaScriptCore/ChangeLog:60 > + (JSC::SourceProviderCacheItem::endArrowFunctionToken): The previous one remains here. Let's drop it :) > Source/JavaScriptCore/ChangeLog:106 > + (JSC::SyntaxChecker::setFunctionNameStart): Ah, when using `Tools/Scripts/webkit-patch upload --update-changelogs` and the ChangeLog is significantly different from the previous version, it appends new logs. So let's drop the previous one manually. > Source/JavaScriptCore/parser/Parser.cpp:1270 > + result = parseArrowFunctionExpression(context, parseType); Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"? > Source/JavaScriptCore/parser/Parser.cpp:1447 > + bool isClassConstructor; Let's drop this line. > Source/JavaScriptCore/parser/Parser.cpp:1472 > + isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor; It seems this line is not necessary, right? > Source/JavaScriptCore/parser/Parser.cpp:1478 > + isClassConstructor = constructorKind != ConstructorKind::None; This always override the previous one. And I think this line is not necessary. > Source/JavaScriptCore/parser/Parser.cpp:1487 > + isClassConstructor = false; I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?) > Source/JavaScriptCore/parser/Parser.cpp:-1387 > - bool isClassConstructor = constructorKind != ConstructorKind::None; I think using this is enough because constructorKind for `ArrowFunctionParseType` is always ConstructorKind::None. > Source/JavaScriptCore/parser/Parser.cpp:1495 > + constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None; I think this line is not necessary. > Source/JavaScriptCore/parser/Parser.cpp:1610 > + info.isEndByTerminator = true; OK, they are initialized in struct definition with C++11 initialization form. > Source/JavaScriptCore/parser/Parser.cpp:1635 > + parameters.isPrevTerminator = m_lexer->prevTerminator(); These fields are not initialized if parseType is not ArrowFunctionParseType. Since they are primimtive types (unsigned int etc.), use of uninitialized values causes undefined behavior in C++. Let's take the either way 1. initialize them in the struct definition with C++11 initialization form. 2. initialize them before this if-branch. > Source/JavaScriptCore/parser/ParserFunctionInfo.h:41 > + unsigned startFunctionOffset = 0; Let's exchange this member's order. unsigned startFunctionOffset = 0; unsigned endFunctionOffset = 0; > Source/JavaScriptCore/parser/ParserFunctionInfo.h:49 > + FunctionBodyType functionBodyType = StandardFunctionBodyBlock; They are initialized with C++ initialization form. > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39 > + unsigned endFunctionOffset; Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better. > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50 > + bool isPrevTerminator; Since the above 3 fields not set if the function is standard function, it becomes undefined values in C++. Let's use C++11 initialization. bool isBodyArrowExpression { false }; JSTokenType tokenType { CLOSEBRACE }; bool isPrevTerminator { false };

Created attachment 255398 [details] Patch

Attachment 255398 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255191&action=review >> Source/JavaScriptCore/ChangeLog:106 >> + (JSC::SyntaxChecker::setFunctionNameStart): > > Ah, when using `Tools/Scripts/webkit-patch upload --update-changelogs` and the ChangeLog is significantly different from the previous version, it appends new logs. > So let's drop the previous one manually. Done >> Source/JavaScriptCore/parser/Parser.cpp:1270 >> + result = parseArrowFunctionExpression(context, parseType); > > Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"? Done. >> Source/JavaScriptCore/parser/Parser.cpp:1447 >> + bool isClassConstructor; > > Let's drop this line. Done >> Source/JavaScriptCore/parser/Parser.cpp:1472 >> + isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor; > > It seems this line is not necessary, right? Done >> Source/JavaScriptCore/parser/Parser.cpp:1478 >> + isClassConstructor = constructorKind != ConstructorKind::None; > > This always override the previous one. And I think this line is not necessary. Done >> Source/JavaScriptCore/parser/Parser.cpp:1487 >> + isClassConstructor = false; > > I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?) Done >> Source/JavaScriptCore/parser/Parser.cpp:1495 >> + constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None; > > I think this line is not necessary. Done >> Source/JavaScriptCore/parser/Parser.cpp:1610 >> + info.isEndByTerminator = true; > > OK, they are initialized in struct definition with C++11 initialization form. Done >> Source/JavaScriptCore/parser/Parser.cpp:1635 >> + parameters.isPrevTerminator = m_lexer->prevTerminator(); > > These fields are not initialized if parseType is not ArrowFunctionParseType. > Since they are primimtive types (unsigned int etc.), use of uninitialized values causes undefined behavior in C++. > > Let's take the either way > 1. initialize them in the struct definition with C++11 initialization form. > 2. initialize them before this if-branch. So I've added initializing of this paramters in the struct definition >> Source/JavaScriptCore/parser/ParserFunctionInfo.h:41 >> + unsigned startFunctionOffset = 0; > > Let's exchange this member's order. > unsigned startFunctionOffset = 0; > unsigned endFunctionOffset = 0; Done >> Source/JavaScriptCore/parser/ParserFunctionInfo.h:49 >> + FunctionBodyType functionBodyType = StandardFunctionBodyBlock; > > They are initialized with C++ initialization form. Done >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39 >> + unsigned endFunctionOffset; > > Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better. Done >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50 >> + bool isPrevTerminator; > > Since the above 3 fields not set if the function is standard function, it becomes undefined values in C++. > Let's use C++11 initialization. > > bool isBodyArrowExpression { false }; > JSTokenType tokenType { CLOSEBRACE }; > bool isPrevTerminator { false }; Done

Comment on attachment 255398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255398&action=review > Source/JavaScriptCore/parser/Parser.cpp:2055 > + failIfFalse(lhs, "Cannot parse expression"); Here, ^Y control sequence is inserted.

Created attachment 255400 [details] Patch

Attachment 255400 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 255401 [details] Patch

Created attachment 255426 [details] Patch

Attachment 255426 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255426&action=review > Source/JavaScriptCore/parser/Parser.cpp:355 > + while (TreeStatement statement = parseStatement(context, directive, &directiveLiteralLength, functionParseType)) { I think using `parseStatement` for ArrowFunctionBodyExpression case is not good. Now we have information about it as `functionParseType`. so instead of using parseArrowFunctionSingleExpressionBody inside parseStatement, just separate path here and use parseArrowFunctionSingleExpressionBody directly. > Source/JavaScriptCore/parser/Parser.cpp:593 > + failDueToUnexpectedToken(); Why this is needed? > Source/JavaScriptCore/parser/Parser.cpp:615 > + next(); I think `parseType` is always ArrowFunctionParseType. So this is not necessary. > Source/JavaScriptCore/parser/Parser.cpp:1226 > +#endif Drop here. > Source/JavaScriptCore/parser/Parser.cpp:1276 > +#endif Move this into parseSourceElements (at that time, do not loop.) So, parseStatement's modification is dropped. > Source/JavaScriptCore/parser/Parser.cpp:1505 > + if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) { We lookup previously parsed function with info.startFunctionOffset key. But this value is later changed for ArrowFunctionBodyBlock case. So when the type is ArrowFunctionBodyBlock, cachedInfo is never hit. I've applied this patch to my workingcopy and log the searched `info.startFunctionOffset` and stored offset. To test it, be carefull about the function's length. If the length is less than or equal to minimumFunctionLengthToCache, cachedInfo is not created. > Source/JavaScriptCore/parser/Parser.cpp:1575 > + info.startFunctionOffset = m_token.m_data.offset; Because of this change, ArrowFunctionBodyBlock cache is never hit. > Source/JavaScriptCore/parser/Parser.cpp:1620 > + unsigned endFunctionOffset = location.startOffset; Here, we just use location.startOffset. Is it OK for ArrowFunctionBodyExpression ended with line terminator? (we set info.endFunctionOffset = location.endOffset in the previous block). I think dropping this is better to keep consistency with info.endFunctionOffset. > Source/JavaScriptCore/parser/Parser.cpp:1634 > + parameters.endFunctionStartOffset = endFunctionOffset; Use info.endFunctionOffset. > Source/JavaScriptCore/parser/Parser.cpp:2059 > + return parseArrowFunctionWrapper(context); Now, PrimaryExpression part is removed. I think `parseArrowFunctionWrapper` can be renamed to `parseArrowFunctionExpression`. > Source/JavaScriptCore/parser/Parser.cpp:2846 > + failIfFalse(match(IDENT) || match(OPENPAREN), "Expected an arrow function input parameter"); This condition is guaranteed by `isArrowFunctionParameters()`. So this can be ASSERT. > Source/JavaScriptCore/parser/SourceCode.h:128 > + ASSERT(endArrowFunction >= 1); OK, now you updated `endArrowFunction`. So this assertion should be removed, correct?

Created attachment 255506 [details] Patch

Attachment 255506 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255426&action=review >> Source/JavaScriptCore/parser/Parser.cpp:355 >> + while (TreeStatement statement = parseStatement(context, directive, &directiveLiteralLength, functionParseType)) { > > I think using `parseStatement` for ArrowFunctionBodyExpression case is not good. > Now we have information about it as `functionParseType`. > so instead of using parseArrowFunctionSingleExpressionBody inside parseStatement, just separate path here and use parseArrowFunctionSingleExpressionBody directly. Done >> Source/JavaScriptCore/parser/Parser.cpp:593 >> + failDueToUnexpectedToken(); > > Why this is needed? It is raise exception for following statement '=>{a}' This function can be run in several cases: 1) from parseFunctionBody - correct path 2) during reparsing - correct path 3) from parseSourceElements, when js has nor correct statement '=>{ return a}' - wrong path >> Source/JavaScriptCore/parser/Parser.cpp:615 >> + next(); > > I think `parseType` is always ArrowFunctionParseType. So this is not necessary. In case of reparsing parseType there will be StandardFunctionParseType >> Source/JavaScriptCore/parser/Parser.cpp:1226 >> +#endif > > Drop here. Done >> Source/JavaScriptCore/parser/Parser.cpp:1276 >> +#endif > > Move this into parseSourceElements (at that time, do not loop.) > So, parseStatement's modification is dropped. Done >> Source/JavaScriptCore/parser/Parser.cpp:1505 >> + if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) { > > We lookup previously parsed function with info.startFunctionOffset key. But this value is later changed for ArrowFunctionBodyBlock case. > So when the type is ArrowFunctionBodyBlock, cachedInfo is never hit. > I've applied this patch to my workingcopy and log the searched `info.startFunctionOffset` and stored offset. > > To test it, be carefull about the function's length. If the length is less than or equal to minimumFunctionLengthToCache, cachedInfo is not created. I've moved up logic of the checking type of arrow function and setting the info.startFunctionOffset. >> Source/JavaScriptCore/parser/Parser.cpp:1575 >> + info.startFunctionOffset = m_token.m_data.offset; > > Because of this change, ArrowFunctionBodyBlock cache is never hit. Moved before looking up to cache >> Source/JavaScriptCore/parser/Parser.cpp:1620 >> + unsigned endFunctionOffset = location.startOffset; > > Here, we just use location.startOffset. Is it OK for ArrowFunctionBodyExpression ended with line terminator? (we set info.endFunctionOffset = location.endOffset in the previous block). > I think dropping this is better to keep consistency with info.endFunctionOffset. There is no tocken line terminator. So I end offset + 1 position of the tocken before line terminator. >> Source/JavaScriptCore/parser/Parser.cpp:1634 >> + parameters.endFunctionStartOffset = endFunctionOffset; > > Use info.endFunctionOffset. Done >> Source/JavaScriptCore/parser/Parser.cpp:2059 >> + return parseArrowFunctionWrapper(context); > > Now, PrimaryExpression part is removed. I think `parseArrowFunctionWrapper` can be renamed to `parseArrowFunctionExpression`. Done >> Source/JavaScriptCore/parser/Parser.cpp:2846 >> + failIfFalse(match(IDENT) || match(OPENPAREN), "Expected an arrow function input parameter"); > > This condition is guaranteed by `isArrowFunctionParameters()`. So this can be ASSERT. Done. >> Source/JavaScriptCore/parser/SourceCode.h:128 >> + ASSERT(endArrowFunction >= 1); > > OK, now you updated `endArrowFunction`. So this assertion should be removed, correct? Correct. It always greater than startArrowFunction and 1. Done

Comment on attachment 255506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255506&action=review Great. I think the current token modification code has several issues. I suggest one solution. Please point it if there's an issue. > Source/JavaScriptCore/parser/Parser.cpp:358 > + TreeStatement arrowfunctionStatement = parseArrowFunctionSingleExpressionBody(context, functionParseType); OK, I understand. This inner branch is intended to be used for ArrowFunctionExpression + ExpressionBody case because in body case ((x) => { } case), ARROWFUNCTION token is already consumed. However, in reparsing phase, functionParseType becomes StandardFunctionParseType (I think it should be fixed in the separated patch). The problematic case is that, { => x; } This is avoided because, 1. If the current phase is not reparsing phase, functionParseType is restricted to arrow function parse type. 2. If the current phase is reparsing phase, these code is once parsed in non reparsing phase. In reparsing phase, there's no syntax error. So above case is already avoided in the non reparsing phase. > Source/JavaScriptCore/parser/Parser.cpp:365 > + if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) { Why this branch is needed? Is it possible to write just the following here? propagateError(); return sourceElements; Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed. > Source/JavaScriptCore/parser/Parser.cpp:368 > + } Is here's fall through correct? > Source/JavaScriptCore/parser/Parser.cpp:610 > + failDueToUnexpectedToken(); OK, please insert comment about reparsing phase and parseType relation like the following. // When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function. // This condition considers the following situations. // (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error. // (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function. > Source/JavaScriptCore/parser/Parser.cpp:631 > + if (parseType == StandardFunctionParseType) Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential. BTW, this code path is a little bit complicated. I recommend to insert comment here. // Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code. // So consuming the last token here is needed. Correct? > Source/JavaScriptCore/parser/Parser.cpp:1567 > + } Drop them. > Source/JavaScriptCore/parser/Parser.cpp:1574 > + next(); Drop this. > Source/JavaScriptCore/parser/Parser.cpp:1590 > + info.startFunctionOffset = m_token.m_data.offset; This line is not needed since it's already set. I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that. > Source/JavaScriptCore/parser/Parser.cpp:1625 > + info.endFunctionOffset = location.endOffset + 1; Modifying offset is dangerous I think. It easily leads heap overflow. And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information. It ensures the following invariant. 1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored. And when recovering from it, always consume the token. > Source/JavaScriptCore/parser/Parser.cpp:1629 > + info.isEndByTerminator = m_lexer->prevTerminator(); Drop isEndByTerminator information. > Source/JavaScriptCore/parser/Parser.cpp:1647 > + parameters.endFunctionEndOffset = endFunctionEndOffset; In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`. So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended? > Source/JavaScriptCore/parser/Parser.cpp:1663 > + if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) { Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.

Hm, I think modifying index (like endArrowFunction + 1) is not good since it leads heap overrun easily. So I suggest the following change, GSkachkov, what do you think about it? 1. info.endFunctionOffset is stored independently from the lastToken information. When cacheInfo is hit, just set info.endFunctionOffset = cachedInfo->endFunctionOffset. Not recalculate it. 2. cachedInfo (and param) has lastToken information. Change endFunctionStartOffset etc. to lastTokenStartOffset etc. And endFunctionToken is generated from this information. 3. Store the last token information to cacheInfo. Don't separate pathes for Arrow+Expr, Arrow+Block and Standard. In all cases, just store the last token. And info.endFunctionOffset is adjusted only in ArrowExpr case (lastToken.endOffset). In the other case, use lastToken.startOffset as info.endFunctionOffset. And when using cachedInfo, just set stored token and call `next()` in all cases. 4. Not containing the arrow function expression body's terminator token (like ';') in function code. `next()` in reparsing phase in SingleExpression parsing is removed. This drops many if-defs, simplifies the code and avoids index modification (like endxxx + 1).

Created attachment 255586 [details] Patch

Comment on attachment 255506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255506&action=review >> Source/JavaScriptCore/parser/Parser.cpp:365 >> + if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) { > > Why this branch is needed? > Is it possible to write just the following here? > > propagateError(); > return sourceElements; > > Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed. Good shoot, before it was necessary. Done. >> Source/JavaScriptCore/parser/Parser.cpp:368 >> + } > > Is here's fall through correct? I've removed propagateError() statement >> Source/JavaScriptCore/parser/Parser.cpp:610 >> + failDueToUnexpectedToken(); > > OK, please insert comment about reparsing phase and parseType relation like the following. > > // When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function. > // This condition considers the following situations. > // (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error. > // (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function. Done >> Source/JavaScriptCore/parser/Parser.cpp:631 >> + if (parseType == StandardFunctionParseType) > > Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential. > > BTW, this code path is a little bit complicated. I recommend to insert comment here. > > // Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code. > // So consuming the last token here is needed. > > Correct? I'm not sure. It is possible when state of lexer is reparsing because of reparsing of wrapped function, but not arrow function. For instance test with (function funcSelfExecAE1(value) { var f = x => x+1; return f(value);})(123); fails because parser is reparsing funcSelfExecAE1 function. Anyway I get rid of this condition, after refactoring of the endFunctionOffset calculation >> Source/JavaScriptCore/parser/Parser.cpp:1567 >> + } > > Drop them. Not sure that I need to do this. I've made small refactoring those line possible it will be more clear what is going on in this lines. >> Source/JavaScriptCore/parser/Parser.cpp:1590 >> + info.startFunctionOffset = m_token.m_data.offset; > > This line is not needed since it's already set. > I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that. The line above is moving to next token, so we need reset info.startFunctionOffset again. I've made small refactoring of this conditions to avoid nested conditions, hope it will be more readable. >> Source/JavaScriptCore/parser/Parser.cpp:1625 >> + info.endFunctionOffset = location.endOffset + 1; > > Modifying offset is dangerous I think. It easily leads heap overflow. > And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. > > I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information. > It ensures the following invariant. > > 1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored. > > And when recovering from it, always consume the token. OK. Agree, done >> Source/JavaScriptCore/parser/Parser.cpp:1629 >> + info.isEndByTerminator = m_lexer->prevTerminator(); > > Drop isEndByTerminator information. Done >> Source/JavaScriptCore/parser/Parser.cpp:1647 >> + parameters.endFunctionEndOffset = endFunctionEndOffset; > > In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`. > So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended? Fixed and used endFunctionEndOffset variables. >> Source/JavaScriptCore/parser/Parser.cpp:1663 >> + if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) { > > Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`. Done

Attachment 255586 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 255589 [details] Patch

Created attachment 255593 [details] Patch

(In reply to comment #82) > Hm, I think modifying index (like endArrowFunction + 1) is not good since it > leads heap overrun easily. > > So I suggest the following change, GSkachkov, what do you think about it? > > 1. info.endFunctionOffset is stored independently from the lastToken > information. When cacheInfo is hit, just set info.endFunctionOffset = > cachedInfo->endFunctionOffset. Not recalculate it. > > 2. cachedInfo (and param) has lastToken information. Change > endFunctionStartOffset etc. to lastTokenStartOffset etc. And > endFunctionToken is generated from this information. > > 3. Store the last token information to cacheInfo. Don't separate pathes for > Arrow+Expr, Arrow+Block and Standard. In all cases, just store the last > token. And info.endFunctionOffset is adjusted only in ArrowExpr case > (lastToken.endOffset). In the other case, use lastToken.startOffset as > info.endFunctionOffset. And when using cachedInfo, just set stored token and > call `next()` in all cases. > > 4. Not containing the arrow function expression body's terminator token > (like ';') in function code. `next()` in reparsing phase in SingleExpression > parsing is removed. > > This drops many if-defs, simplifies the code and avoids index modification > (like endxxx + 1). I've tried to implement most of your suggestion in last patch. Please take a look if it code is better

Attachment 255593 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 255506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255506&action=review Comments. >>> Source/JavaScriptCore/parser/Parser.cpp:368 >>> + } >> >> Is here's fall through correct? > > I've removed propagateError() statement Oh, why is it? I think propagateError(); return sourceElements; is needed here. To fail with { => expr; } correctly.

Comment on attachment 255593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255593&action=review I think only a few nits remains. Code becomes very simple, nice. Thank you for your contributions. Sorry for troubling you by a long time review. > Source/JavaScriptCore/parser/Parser.cpp:364 > + I think propagateError() is needed before `return sourceElements`. > Source/JavaScriptCore/parser/Parser.cpp:1529 > + endLocation.lineStartOffset = cachedInfo->lastTockenLineStartOffset; Looks nice. > Source/JavaScriptCore/parser/Parser.cpp:1570 > + } OK, looks nice. > Source/JavaScriptCore/parser/Parser.cpp:1588 > + info.startFunctionOffset = m_token.m_data.offset; Still I don't understand why this line is needed. `info.startFunctionOffset` is already adjusted in the `info.startFunctionOffset = (info.functionBodyType == ArrowFunctionBodyBlock) ? m_token.m_data.offset : info.arrowFunctionOffset;` part, right? When setting info.startFunctionOffset previously, we already considered functionBodyType. When block body functions come, we set `{`'s offset. And when expr body functions come, we set `=>`'s offset. So this resetting is already done. Anyway, if info.startFunctionOffset is modified after looking up the cacheInfo, cacheInfo's key becomes different. So if this adjustment is needed, this need to be done before `if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {` part. Reaching here, we already looked up the cache with the previous info.startFunctionOffset. > Source/JavaScriptCore/parser/Parser.cpp:1628 > +#endif Very nice :D

Created attachment 255616 [details] Patch

Comment on attachment 255593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255593&action=review >> Source/JavaScriptCore/parser/Parser.cpp:1588 >> + info.startFunctionOffset = m_token.m_data.offset; > > Still I don't understand why this line is needed. > `info.startFunctionOffset` is already adjusted in the > `info.startFunctionOffset = (info.functionBodyType == ArrowFunctionBodyBlock) ? m_token.m_data.offset : info.arrowFunctionOffset;` > part, right? > > When setting info.startFunctionOffset previously, we already considered functionBodyType. > When block body functions come, we set `{`'s offset. And when expr body functions come, we set `=>`'s offset. > So this resetting is already done. > > Anyway, if info.startFunctionOffset is modified after looking up the cacheInfo, cacheInfo's key becomes different. > So if this adjustment is needed, this need to be done before `if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {` part. Reaching here, we already looked up the cache with the previous info.startFunctionOffset. Now I got it :-) The same was done in line 1506. Fixed.

Attachment 255616 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 255617 [details] Patch

Attachment 255617 [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] Total errors found: 1 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #91) > Comment on attachment 255593 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255593&action=review > > I think only a few nits remains. Code becomes very simple, nice. > Thank you for your contributions. Sorry for troubling you by a long time > review. > > > Source/JavaScriptCore/parser/Parser.cpp:364 > > + > > I think propagateError() is needed before `return sourceElements`. > Returned back

Comment on attachment 255617 [details] Patch r=me. Thank you!

Comment on attachment 255617 [details] Patch Clearing flags on attachment: 255617 Committed r185989: <http://trac.webkit.org/changeset/185989>

All reviewed patches have been landed. Closing bug.