Bug 153981

Created attachment 271198 [details] Patch Patch

Created attachment 271327 [details] Patch Tiny fixes

Comment on attachment 271327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271327&action=review LGTM, just a few design suggestions and renaming suggestions. Have you run perf numbers? > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132 > + bool isInnerArrowFunctionUseArguments() { return m_arrowFunctionCodeFeatures & ArgumentsArrowFunctionFeature; } > + bool isInnerArrowFunctionUseSuperCall() { return m_arrowFunctionCodeFeatures & SuperCallArrowFunctionFeature; } > + bool isInnerArrowFunctionUseSuperProperty() { return m_arrowFunctionCodeFeatures & SuperPropertyArrowFunctionFeature; } > + bool isInnerArrowFunctionUseEval() { return m_arrowFunctionCodeFeatures & EvalArrowFunctionFeature; } > + bool isInnerArrowFunctionUseThis() { return m_arrowFunctionCodeFeatures & ThisArrowFunctionFeature; } > + bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; } I would name these like "doAnyInnerArrowFunctionsX" i.e: isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051 > +bool BytecodeGenerator::isNewTargetUsedInArrowFunction() > +{ > + return m_codeBlock->isInnerArrowFunctionUseNewTarget() || m_codeBlock->isInnerArrowFunctionUseSuperCall(); > +} Is "new.target" not allowed in eval? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648 > + void emitPutThisToArrowFunctionContextScope(bool = false); Style: give this bool a name, like "shouldAlwaysPut" or something. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724 > + bool isThisUsedInArrowFunction(); > + bool isNewTargetUsedInArrowFunction(); > + bool isSuperUsedInArrowFunction(); > + bool isArgumentsUsedInArrowFunction(); nit: I would name these with "Inner" before "In" like: "isThisUsedInInnerArrowFunction" > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764 > + generator.emitPutThisToArrowFunctionContextScope(true); Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function: ``` bool forcePutToScope = true; generator.emitPutThisToArrowFunctionContextScope(forcePutToScope); ``` > Source/JavaScriptCore/parser/Parser.cpp:2067 > + if (functionBodyType != StandardFunctionBodyBlock) > + closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall(); An alternative to repeatedly doing a scope search here is to just propagate this information when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal. Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock" > Source/JavaScriptCore/parser/Parser.cpp:3644 > + if (m_vm->propertyNames->eval == *ident) > + closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseEval(); > + else if (m_vm->propertyNames->arguments == *ident) > + closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns(); If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables. > Source/JavaScriptCore/parser/ParserModes.h:164 > +typedef uint16_t ArrowFunctionCodeFeatures; this can be uint8_t > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47 > + unsigned innerArrowFunctionFeatures; Why not ArrowFunctionCodeFeatures instead of unsigned? > Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92 > + unsigned innerArrowFunctionFeatures : 31; ditto.

Created attachment 271677 [details] Patch Fix comments

Comment on attachment 271327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271327&action=review >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132 >> + bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; } > > I would name these like "doAnyInnerArrowFunctionsX" > i.e: > isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments Renamed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051 >> +} > > Is "new.target" not allowed in eval? Ups. I forget about eval. Fixed. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648 >> + void emitPutThisToArrowFunctionContextScope(bool = false); > > Style: give this bool a name, like "shouldAlwaysPut" or something. added >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724 >> + bool isArgumentsUsedInArrowFunction(); > > nit: I would name these with "Inner" before "In" > like: > "isThisUsedInInnerArrowFunction" Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764 >> + generator.emitPutThisToArrowFunctionContextScope(true); > > Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function: > ``` > bool forcePutToScope = true; > generator.emitPutThisToArrowFunctionContextScope(forcePutToScope); > ``` Done >> Source/JavaScriptCore/parser/Parser.cpp:2067 >> + closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall(); > > An alternative to repeatedly doing a scope search here is to just propagate this information > when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal. > Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read > with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock" Refactored to use popScope and fixed condition >> Source/JavaScriptCore/parser/Parser.cpp:3644 >> + closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns(); > > If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables. Done >> Source/JavaScriptCore/parser/ParserModes.h:164 >> +typedef uint16_t ArrowFunctionCodeFeatures; > > this can be uint8_t Changed >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47 >> + unsigned innerArrowFunctionFeatures; > > Why not ArrowFunctionCodeFeatures instead of unsigned? Fixed >> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92 >> + unsigned innerArrowFunctionFeatures : 31; > > ditto. Done

(In reply to comment #3) > Comment on attachment 271327 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271327&action=review > > LGTM, just a few design suggestions and renaming suggestions. > Have you run perf numbers? > Not yet, I'm going to run it tomorrow. I think we need to add several more benchmark tests with eval and classes

Created attachment 271685 [details] Patch Fix Changelog & remove commented out code

Comment on attachment 271685 [details] Patch Found bugs in using arrow function inside of the constructor

Created attachment 271811 [details] Patch Fix bugs: putting 'this' to arrow function context

Comment on attachment 271811 [details] Patch Attachment 271811 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/856427 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html js/regress/arrowfunction-call-in-class-method.html js/regress/arrowfunction-call-in-function.html

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

Comment on attachment 271811 [details] Patch Attachment 271811 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/856428 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html js/regress/arrowfunction-call-in-class-method.html js/regress/arrowfunction-call-in-function.html

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

Comment on attachment 271811 [details] Patch Attachment 271811 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/856430 New failing tests: js/regress/arrowfunction-call-in-class-constructor.html js/regress/arrowfunction-call-in-class-method.html js/regress/arrowfunction-call-in-function.html

Created attachment 271823 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 271852 [details] Benchmark test result See Benchmark test result in attach

(In reply to comment #3) > Comment on attachment 271327 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271327&action=review > > LGTM, just a few design suggestions and renaming suggestions. > Have you run perf numbers? > I think it quite good figures for arrow function: before_patch after_patch arrowfunction-call-in-class-constructor 300.1710+-13.5061 ^ 191.4837+-4.5846 ^ definitely 1.5676x faster arrowfunction-call-in-class-method 91.2163+-7.7129 ^ 29.3135+-10.2887 ^ definitely 3.1118x faster arrowfunction-call-in-function 96.5411+-31.9533 ^ 19.6813+-1.5141 ^ definitely 4.9052x faster arrowfunction-call 33.9300+-3.3111 ^ 27.0056+-0.9594 ^ definitely 1.2564x faster

Created attachment 271854 [details] Patch Fix tests

Comment on attachment 271854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271854&action=review r=me w/ comments. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050 > + return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval(); I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055 > + return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); Also, 'super' isn't allowed inside an eval. It's a SyntaxError. > Source/JavaScriptCore/parser/Parser.h:473 > + if (m_isArrowFunction && m_usesEval) > + setInnerArrowFunctionUseEval(); > + > + if (m_isArrowFunction && m_vm->propertyNames->arguments == *ident) > + setInnerArrowFunctionUseArgumetns(); An alternative to doing this at every useVariable(.) call is to just do it once when popScope happens. > Source/JavaScriptCore/parser/Parser.h:953 > + // We do not up arrow function features for ordinary function comment not needed.

(In reply to comment #17) > (In reply to comment #3) > > Comment on attachment 271327 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=271327&action=review > > > > LGTM, just a few design suggestions and renaming suggestions. > > Have you run perf numbers? > > > > I think it quite good figures for arrow function: > before_patch > after_patch > arrowfunction-call-in-class-constructor 300.1710+-13.5061 ^ > 191.4837+-4.5846 ^ definitely 1.5676x faster > arrowfunction-call-in-class-method 91.2163+-7.7129 ^ > 29.3135+-10.2887 ^ definitely 3.1118x faster > arrowfunction-call-in-function 96.5411+-31.9533 ^ > 19.6813+-1.5141 ^ definitely 4.9052x faster > arrowfunction-call 33.9300+-3.3111 ^ > 27.0056+-0.9594 ^ definitely 1.2564x faster Nice. It would be worthwhile to open a bug to rewrite the deltablue benchmark (or some other benchmark that tests function calling) to use arrow functions everywhere. That way we can compare that performance to the performance of just using ordinary functions. And we can track performance in arrow functions w/ that benchmark.

Created attachment 271939 [details] Patch Fix last comments

Comment on attachment 271854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271854&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050 >> + return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval(); > > I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review. Fixed >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055 >> + return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval(); > > Also, 'super' isn't allowed inside an eval. It's a SyntaxError. I fixed. However we have issue where will fix this SyntaxError :-) >> Source/JavaScriptCore/parser/Parser.h:473 >> + setInnerArrowFunctionUseArgumetns(); > > An alternative to doing this at every useVariable(.) call > is to just do it once when popScope happens. OK. Refactored. >> Source/JavaScriptCore/parser/Parser.h:953 >> + // We do not up arrow function features for ordinary function > > comment not needed. Removed

(In reply to comment #20) > (In reply to comment #17) > > (In reply to comment #3) > > > Comment on attachment 271327 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=271327&action=review > > > > > > LGTM, just a few design suggestions and renaming suggestions. > > > Have you run perf numbers? > > > > > > > I think it quite good figures for arrow function: > > before_patch > > after_patch > > arrowfunction-call-in-class-constructor 300.1710+-13.5061 ^ > > 191.4837+-4.5846 ^ definitely 1.5676x faster > > arrowfunction-call-in-class-method 91.2163+-7.7129 ^ > > 29.3135+-10.2887 ^ definitely 3.1118x faster > > arrowfunction-call-in-function 96.5411+-31.9533 ^ > > 19.6813+-1.5141 ^ definitely 4.9052x faster > > arrowfunction-call 33.9300+-3.3111 ^ > > 27.0056+-0.9594 ^ definitely 1.2564x faster > > Nice. > > It would be worthwhile to open a bug to rewrite the deltablue benchmark > (or some other benchmark that tests function calling) to use > arrow functions everywhere. That way we can compare that performance > to the performance of just using ordinary functions. And we can track > performance in arrow functions w/ that benchmark. I"ve added your comment to the issue that related to arrow function benchmark tests https://bugs.webkit.org/show_bug.cgi?id=148034

Comment on attachment 271939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271939&action=review > Source/JavaScriptCore/parser/Parser.h:493 > + void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; } I missed this earlier, but the spelling is wrong here.

Created attachment 271952 [details] Patch Fix TYPO

Comment on attachment 271939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271939&action=review >> Source/JavaScriptCore/parser/Parser.h:493 >> + void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; } > > I missed this earlier, but the spelling is wrong here. Oh, sorry. Fixed.

Comment on attachment 271952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271952&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010 > +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier) This type should be "const Identfifier&"

Created attachment 272107 [details] Patch Patch comming

(In reply to comment #27) > Comment on attachment 271952 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271952&action=review > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010 > > +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier) > > This type should be "const Identfifier&" Fixed

Comment on attachment 272107 [details] Patch Clearing flags on attachment: 272107 Committed r197033: <http://trac.webkit.org/changeset/197033>

All reviewed patches have been landed. Closing bug.

This change may have caused 14 new JSC test failures ** The following JSC stress test failures have been introduced: stress/proxy-call.js.always-trigger-copy-phase stress/proxy-call.js.default stress/proxy-call.js.default-ftl stress/proxy-call.js.dfg-eager stress/proxy-call.js.dfg-eager-no-cjit-validate stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit stress/proxy-call.js.ftl-eager stress/proxy-call.js.ftl-eager-no-cjit stress/proxy-call.js.ftl-no-cjit-no-inline-validate stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate stress/proxy-call.js.ftl-no-cjit-small-pool stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler stress/proxy-call.js.no-cjit-validate-phases stress/proxy-call.js.no-llint <https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore-test/logs/stdio>

(In reply to comment #32) > This change may have caused 14 new JSC test failures > > ** The following JSC stress test failures have been introduced: > stress/proxy-call.js.always-trigger-copy-phase > stress/proxy-call.js.default > stress/proxy-call.js.default-ftl > stress/proxy-call.js.dfg-eager > stress/proxy-call.js.dfg-eager-no-cjit-validate > stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit > stress/proxy-call.js.ftl-eager > stress/proxy-call.js.ftl-eager-no-cjit > stress/proxy-call.js.ftl-no-cjit-no-inline-validate > stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate > stress/proxy-call.js.ftl-no-cjit-small-pool > stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler > stress/proxy-call.js.no-cjit-validate-phases > stress/proxy-call.js.no-llint > > <https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore- > test/logs/stdio> It did. I'm looking into a fix now.

Re-opened since this is blocked by bug 154649

Created attachment 272326 [details] Patch Fixed version

Comment on attachment 272326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272326&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575 > + if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction())) Is this change here correct? Does it work for: class A { getValue() { ... } } class B extends A { constructor() { arr = ()=>super.getValue(); arr(); super(); }}

It should work, we emit TDZ always when access to the super. https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922

(In reply to comment #36) > Comment on attachment 272326 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272326&action=review > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575 > > + if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction())) > > Is this change here correct? > Does it work for: > class A { getValue() { ... } } > class B extends A { constructor() { arr = ()=>super.getValue(); arr(); > super(); }} It should work, we emit TDZ always when access to the super. https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922

Comment on attachment 272326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272326&action=review > Source/JavaScriptCore/parser/Parser.cpp:3890 > + if (currentFunctionScope()->isArrowFunction()) { > + if (currentFunctionScope()->hasDirectSuper()) > + currentFunctionScope()->setInnerArrowFunctionUseSuperCall(); > + > + if (currentFunctionScope()->needsSuperBinding()) > + currentFunctionScope()->setInnerArrowFunctionUseSuperProperty(); > + } > + Main change to fix error in tests

Created attachment 272424 [details] Patch Fix comments & Stress tests are executed

Comment on attachment 272424 [details] Patch Attachment 272424 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/892438 New failing tests: storage/indexeddb/key-generator.html

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

Comment on attachment 272424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review > Source/JavaScriptCore/parser/Parser.cpp:3883 > + if (currentFunctionScope()->isArrowFunction()) { I don't understand this code. Why does this happen unconditionally? Shouldn't it depend on if "baseIsSuper == true"? Also, does this miss the case where you just have "super" without a property access or function call afterward? I.e: ()=>super;

Comment on attachment 272424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review > Source/JavaScriptCore/parser/Parser.cpp:3883 > + if (currentFunctionScope()->isArrowFunction()) { I don't understand this code. Why does this happen unconditionally? Shouldn't it depend on if "baseIsSuper == true"? Also, does this miss the case where you just have "super" without a property access or function call afterward? I.e: ()=>super;

Comment on attachment 272424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review > Source/JavaScriptCore/parser/Parser.cpp:3883 > + if (currentFunctionScope()->isArrowFunction()) { I don't understand this code. Why does this happen unconditionally? Shouldn't it depend on if "baseIsSuper == true"? Also, does this miss the case where you just have "super" without a property access or function call afterward? I.e: ()=>super;

Comment on attachment 272424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review > Source/JavaScriptCore/parser/Parser.cpp:3883 > + if (currentFunctionScope()->isArrowFunction()) { I don't understand this code. Why does this happen unconditionally? Shouldn't it depend on if "baseIsSuper == true"? Also, does this miss the case where you just have "super" without a property access or function call afterward? I.e: ()=>super;

(Ah, sorry for the repeated same comment. Bugzilla doesn't like me today.)

Created attachment 272456 [details] Patch Update patch

Comment on attachment 272424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review >>>>> Source/JavaScriptCore/parser/Parser.cpp:3883 >>>>> + if (currentFunctionScope()->isArrowFunction()) { >>>> >>>> I don't understand this code. Why does this happen unconditionally? >>>> Shouldn't it depend on if "baseIsSuper == true"? >>>> Also, does this miss the case where you just have "super" without >>>> a property access or function call afterward? >>>> I.e: >>>> ()=>super; >>> >>> I don't understand this code. Why does this happen unconditionally? >>> Shouldn't it depend on if "baseIsSuper == true"? >>> Also, does this miss the case where you just have "super" without >>> a property access or function call afterward? >>> I.e: >>> ()=>super; >> >> I don't understand this code. Why does this happen unconditionally? >> Shouldn't it depend on if "baseIsSuper == true"? >> Also, does this miss the case where you just have "super" without >> a property access or function call afterward? >> I.e: >> ()=>super; > > I don't understand this code. Why does this happen unconditionally? > Shouldn't it depend on if "baseIsSuper == true"? > Also, does this miss the case where you just have "super" without > a property access or function call afterward? > I.e: > ()=>super; My idea was to check hasDirectSuper & needsSuperBinding that is set when baseIsSuper == true by (setNeedsSuperBinding & setHasDirectSuper), but I see you point, that my current solution is not optimal, it is my fault. I'll rewrite. In general '()=>super;' should return syntax error: 'Cannot reference super', and it do.

Comment on attachment 272456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272456&action=review > Source/JavaScriptCore/parser/Parser.h:488 > + void setInnerArrowFunctionUseSuperCall() { m_innerArrowFunctionFeatures |= SuperCallInnerArrowFunctionFeature; } Nit: all of these setInner* should replace "use" with "uses"

Committed r 197296: <http://trac.webkit.org/changeset/197296>

All reviewed patches have been landed. Closing bug.

*** Bug 149933 has been marked as a duplicate of this bug. ***

Comment on attachment 272424 [details] Patch r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.

(In reply to Brady Eidson from comment #54) > Comment on attachment 272424 [details] > Patch > > r-, as this has been pending review for over a year now. It is > near-impossible that this patch still applies to trunk and unlikely to still > be relevant in its current form. Ohh it is already landed patch. Will clear all flags