Patch coming.
<rdar://problem/31051832>
Created attachment 304447 [details] proposed patch.
Comment on attachment 304447 [details] proposed patch. r=me
Thanks for the review. Landed in r213966: <http://trac.webkit.org/r213966>.
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 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 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.