Bug 169647 – BytecodeGenerator should use the same function to determine if it needs to store the DerivedConstructor in an ArrowFunction lexical environment.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/31051832>

[Mark Lam]

Created attachment 304447 [details]
proposed patch.

[Michael Saboff]

Comment on attachment 304447 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r213966: <http://trac.webkit.org/r213966>.

[Saam Barati]

Comment on attachment 304447 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1036
> +    if ((isConstructor() && constructorKind() == ConstructorKind::Extends) || m_codeBlock->isClassContext()) {

I wonder about the first part of this check:
It seems like: (isConstructor() && kind == Extends) => isClassContext
So perhaps checking it is redundant. But I'm basing this off our naming scheme. I'd have to look more closely at the code to know for sure.

[GSkachkov]

Comment on attachment 304447 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1036
>> +    if ((isConstructor() && constructorKind() == ConstructorKind::Extends) || m_codeBlock->isClassContext()) {
> 
> I wonder about the first part of this check:
> It seems like: (isConstructor() && kind == Extends) => isClassContext
> So perhaps checking it is redundant. But I'm basing this off our naming scheme. I'd have to look more closely at the code to know for sure.

Yeah, it seems that condition '(isConstructor() && constructorKind() == ConstructorKind::Extends)' is already covered by m_codeBlock->isClassContext(). 
Unfortunately I'll manage to check this condition only this weekend (currently very busy on job), if it unnecessary I'll create issue and provide patch.

[GSkachkov]

Comment on attachment 304447 [details]
proposed patch.

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1036
>>> +    if ((isConstructor() && constructorKind() == ConstructorKind::Extends) || m_codeBlock->isClassContext()) {
>> 
>> I wonder about the first part of this check:
>> It seems like: (isConstructor() && kind == Extends) => isClassContext
>> So perhaps checking it is redundant. But I'm basing this off our naming scheme. I'd have to look more closely at the code to know for sure.
> 
> Yeah, it seems that condition '(isConstructor() && constructorKind() == ConstructorKind::Extends)' is already covered by m_codeBlock->isClassContext(). 
> Unfortunately I'll manage to check this condition only this weekend (currently very busy on job), if it unnecessary I'll create issue and provide patch.

You are right, isConstructor() && constructorKind() == ConstructorKind::Extends is covered by m_codeBlock->isClassContext().
isClassContext is just  executable->superBinding() == SuperBinding::Needed;
also I've checked Parser.cpp and found out following lines in parserFunctionInfo:
if (m_defaultConstructorKind != ConstructorKind::None) {
    constructorKind = m_defaultConstructorKind;
    expectedSuperBinding = m_defaultConstructorKind == ConstructorKind::Extends ? SuperBinding::Needed : SuperBinding::NotNeeded;
}
From that I made conclusion that function that is constructor already has value isClassContext, so I'll prepare patch to remove condition.