Bug 144954 - Small refactoring before ES6 Arrow function implementation
Summary: Small refactoring before ES6 Arrow function implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 144974
Blocks: 140855 144955
  Show dependency treegraph
 
Reported: 2015-05-13 10:37 PDT by GSkachkov
Modified: 2015-05-14 13:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.67 KB, patch)
2015-05-13 11:11 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2015-05-13 14:01 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2015-05-13 14:09 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (4.19 KB, patch)
2015-05-14 07:52 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-05-13 10:37:05 PDT
It is necessary make small refactoring of parser/Parser.cpp before arrow function implementation.
Comment 1 GSkachkov 2015-05-13 11:11:11 PDT
Created attachment 253037 [details]
Patch
Comment 2 GSkachkov 2015-05-13 12:38:34 PDT
Comment on attachment 253037 [details]
Patch

Small refactoring. Landing of this page allow to move to the base implemention of the arrow function.
Comment 3 Filip Pizlo 2015-05-13 13:31:31 PDT
Comment on attachment 253037 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:2
> +        Small refactoring of the parcer.cpp file. It is necessary to increase readability of the patch

Add a blank line here.

> Source/JavaScriptCore/parser/Parser.cpp:1333
> -
> +    

Revert.
Comment 4 WebKit Commit Bot 2015-05-13 13:33:49 PDT
Comment on attachment 253037 [details]
Patch

Rejecting attachment 253037 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 253037, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/4708680951922688
Comment 5 GSkachkov 2015-05-13 14:01:33 PDT
Created attachment 253058 [details]
Patch
Comment 6 GSkachkov 2015-05-13 14:09:47 PDT
Created attachment 253059 [details]
Patch
Comment 7 WebKit Commit Bot 2015-05-13 15:25:19 PDT
Comment on attachment 253059 [details]
Patch

Clearing flags on attachment: 253059

Committed r184313: <http://trac.webkit.org/changeset/184313>
Comment 8 WebKit Commit Bot 2015-05-13 15:25:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2015-05-13 16:25:50 PDT
Looks like this patch introduced a crash on 32-bit builds:
https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/3967/steps/webkit-32bit-jsc-test/logs/stdio
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/10038/steps/webkit-32bit-jsc-test/logs/stdio

jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: PASS ({get a([x]){}}) threw exception SyntaxError: Unexpected token '['. getter functions must have no parameters..
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: ASSERTION FAILED: !hasError()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: /Volumes/Data/slave/yosemite-32bitJSC-debug/build/Source/JavaScriptCore/parser/Parser.h(823) : JSC::Parser::SavePoint JSC::Parser<JSC::Lexer<unsigned char> >::createSavePoint() [LexerType = JSC::Lexer<unsigned char>]
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 1   0xb1619d WTFCrash
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 2   0x8f7bf2 JSC::Parser<JSC::Lexer<unsigned char> >::createSavePoint()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 3   0x9853b1 JSC::SyntaxChecker::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::SyntaxChecker>(JSC::SyntaxChecker&, JSC::SourceElementsMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 4   0x984cad JSC::ASTBuilder::FunctionBody JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionBody<JSC::ASTBuilder>(JSC::ASTBuilder&, int, int, int, JSC::ConstructorKind)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 5   0x979b8f bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::FunctionRequirements, JSC::FunctionParseMode, bool, JSC::ConstructorKind, JSC::SuperBinding, int, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 6   0x982077 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseGetterSetter<JSC::ASTBuilder>(JSC::ASTBuilder&, bool, JSC::PropertyNode::Type, unsigned int, JSC::ConstructorKind, JSC::SuperBinding)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 7   0x980834 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 8   0x97d562 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseObjectLiteral<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 9   0x976d8f JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 10  0x975400 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 11  0x9740fe JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 12  0x9731eb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 13  0x9726df JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 14  0x971acb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 15  0x97109d JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 16  0x976df0 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 17  0x975400 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 18  0x9740fe JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 19  0x9731eb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 20  0x9726df JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 21  0x971acb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 22  0x97109d JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 23  0x970da7 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionStatement<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 24  0x965fe9 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 25  0x8f47a8 JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 26  0x8f3f0e JSC::Parser<JSC::Lexer<unsigned char> >::parseInner()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 27  0x1dce9c std::__1::unique_ptr<JSC::EvalNode, std::__1::default_delete<JSC::EvalNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::EvalNode>(JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 28  0x1dbdda std::__1::unique_ptr<JSC::EvalNode, std::__1::default_delete<JSC::EvalNode> > JSC::parse<JSC::EvalNode>(JSC::VM*, JSC::SourceCode const&, JSC::FunctionParameters*, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::JSParserCodeType, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind, JSC::ThisTDZMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 29  0x1dab4c JSC::UnlinkedEvalCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedEvalCodeBlock, JSC::EvalExecutable>(JSC::VM&, JSC::EvalExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 30  0x1d918f JSC::CodeCache::getEvalCodeBlock(JSC::VM&, JSC::EvalExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 31  0x76d821 JSC::JSGlobalObject::createEvalCodeBlock(JSC::ExecState*, JSC::EvalExecutable*, JSC::ThisTDZMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: test_script_18968: line 2:  4296 Segmentation fault: 11  "$@" ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true resources/standalone-pre.js object-literal-syntax.js resources/standalone-post.js
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: ERROR: Unexpected exit code: 139
FAIL: jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout
Comment 10 Ryosuke Niwa 2015-05-13 16:29:16 PDT
Actually, it's nothing to do with 32-bit-ness. It's an assertion failure.
Comment 11 WebKit Commit Bot 2015-05-13 16:30:41 PDT
Re-opened since this is blocked by bug 144974
Comment 13 Saam Barati 2015-05-13 16:52:14 PDT
Comment on attachment 253059 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:1314
>  {

It's also weird that we we're returning bools and ints from this function.
I think there is probably some case that isn't being thought through.

> Source/JavaScriptCore/parser/Parser.cpp:1363
> +    int parametersStart = parseFunctionParamters(context, requirements, mode, nameIsInContainingScope, functionScope, info);

You might need to propagate errors here.
Comment 14 GSkachkov 2015-05-14 07:52:45 PDT
Created attachment 253119 [details]
Patch
Comment 15 GSkachkov 2015-05-14 08:06:32 PDT
Comment on attachment 253059 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:1314
>>  {
> 
> It's also weird that we we're returning bools and ints from this function.
> I think there is probably some case that isn't being thought through.

I'm quite new in webkit, so I tried to follow exist examples. Could you please suggest what I should change in this function?

>> Source/JavaScriptCore/parser/Parser.cpp:1363
>> +    int parametersStart = parseFunctionParamters(context, requirements, mode, nameIsInContainingScope, functionScope, info);
> 
> You might need to propagate errors here.

Done in new patch
Comment 16 GSkachkov 2015-05-14 08:09:41 PDT
(In reply to comment #10)
> Actually, it's nothing to do with 32-bit-ness. It's an assertion failure.

I see, my fault. I always run tests in release mode, because in debug mode it is run so slowly on my Laptop.  
Will check in debug mode also before load patch.
Comment 17 WebKit Commit Bot 2015-05-14 13:40:11 PDT
Comment on attachment 253119 [details]
Patch

Clearing flags on attachment: 253119

Committed r184349: <http://trac.webkit.org/changeset/184349>
Comment 18 WebKit Commit Bot 2015-05-14 13:40:16 PDT
All reviewed patches have been landed.  Closing bug.