Bug 169647

Summary: BytecodeGenerator should use the same function to determine if it needs to store the DerivedConstructor in an ArrowFunction lexical environment.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gskachkov, keith_miller, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 169832    
Attachments:
Description Flags
proposed patch. msaboff: review+

Description Mark Lam 2017-03-14 17:03:42 PDT
Patch coming.
Comment 1 Radar WebKit Bug Importer 2017-03-14 17:05:18 PDT
<rdar://problem/31051832>
Comment 2 Mark Lam 2017-03-14 17:12:29 PDT
Created attachment 304447 [details]
proposed patch.
Comment 3 Michael Saboff 2017-03-14 17:15:28 PDT
Comment on attachment 304447 [details]
proposed patch.

r=me
Comment 4 Mark Lam 2017-03-14 17:54:39 PDT
Thanks for the review.  Landed in r213966: <http://trac.webkit.org/r213966>.
Comment 5 Saam Barati 2017-03-15 09:50:11 PDT
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.
Comment 6 GSkachkov 2017-03-16 11:09:15 PDT
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.
Comment 7 GSkachkov 2017-03-17 13:36:44 PDT
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.