[Webkit-unassigned] [Bug 149615] [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "super" property

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149615

--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 267103
  --> https://bugs.webkit.org/attachment.cgi?id=267103
Patch

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

Mostly looks good to me but I have a question and some basic comments.

> Source/JavaScriptCore/ChangeLog:9
> +        inside of the arrow function in case if arrow function is nested in method, 

Might be worth explicitly adding constructor to this list of available contexts inside a class.

> Source/JavaScriptCore/ChangeLog:11
> +        class, lead to wrong type of error, should be SyntaxError and this will be fixed in separete patch.

Do you have a bug open for this? If you don't, you should make one.
You should link to it here

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:39
> +    ExecutableInfo(bool needsActivation, bool usesEval, bool isStrictMode, bool isConstructor, bool isBuiltinFunction, ConstructorKind constructorKind, GeneratorThisMode generatorThisMode, SuperBinding superBinding, SourceParseMode parseMode, DerivedContextType _derivedContextType, bool _isArrowFunctionContext, bool isClassContext)

style: Remove the "_" prefixes.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:70
> +    // method, getter & setter are part of the class, so this functionCodeBlock is part of the class
> +    bool isClassContext = executable->parseMode() == SourceParseMode::MethodMode || executable->parseMode() == SourceParseMode::GetterMode || executable->parseMode() == SourceParseMode::SetterMode;

Is this always correct?
We could have code like this:
```
let x = {
    get y() { return 20; }
};
x.y
```
I'm not sure if this would count as SourceParseMode::GetterMode.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:578
> +

style: revert newline.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:799
> +            bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext && metadata->parseMode() == SourceParseMode::ArrowFunctionMode);

This calculation doesn't seem right to me. Shouldn't we be interested in the fact that we're creating an arrow function or not?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:806
> +                bool isArrowFunctionInClassContext = m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext);
> +                derivedContextType = isArrowFunctionInClassContext ? DerivedContextType::DerivedMethodContext : DerivedContextType::None;

same here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:189
> +

style: revert newline.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:1
> +var testCase = function (actual, expected, message) {

Style: 4-space indent this file.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:158
> +// Fixme: should by check if e instanceof SyntaxError https://bugs.webkit.org/show_bug.cgi?id=150893

Style: "Fixme" => "FIXME" and also indented properly.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:215
> +// FIXME: Problem with access to the super before super in constructor
> +// https://bugs.webkit.org/show_bug.cgi?id=152108
> +//  let j2 = new J(true);
> +//  testCase(j2._value, testValue, 'Error: Some problem with using "super" inside of the constructor');

Style: Indent this comment and all others to be at the level of block they're in.

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:1
> +description('Tests for ES6 arrow function, access to the super property in arrow function');

Style: 4-space indent this file.

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:80
> +//shouldThrow('(new C(false))', 'ReferenceError');

This should be "new C(true)"

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151210/450234d0/attachment.html>