[Webkit-unassigned] [Bug 153981] [ES6] Arrow function syntax. Emit loading&putting this/super only if they are used in arrow function

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 271327
  --> https://bugs.webkit.org/attachment.cgi?id=271327
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.

-- 
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/20160216/8d6df75b/attachment.html>