169647 2017-03-14 17:03:42 -0700 BytecodeGenerator should use the same function to determine if it needs to store the DerivedConstructor in an ArrowFunction lexical environment. 2017-03-17 13:41:46 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 169832 1 mark.lam mark.lam commit-queue gskachkov keith_miller msaboff saam webkit-bug-importer oldest_to_newest 1287888 0 mark.lam 2017-03-14 17:03:42 -0700 Patch coming. 1287891 1 webkit-bug-importer 2017-03-14 17:05:18 -0700 <rdar://problem/31051832> 1287895 2 304447 mark.lam 2017-03-14 17:12:29 -0700 Created attachment 304447 proposed patch. 1287897 3 304447 msaboff 2017-03-14 17:15:28 -0700 Comment on attachment 304447 proposed patch. r=me 1287910 4 mark.lam 2017-03-14 17:54:39 -0700 Thanks for the review. Landed in r213966: <http://trac.webkit.org/r213966>. 1288091 5 304447 saam 2017-03-15 09:50:11 -0700 Comment on attachment 304447 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. 1288620 6 304447 gskachkov 2017-03-16 11:09:15 -0700 Comment on attachment 304447 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. 1289128 7 304447 gskachkov 2017-03-17 13:36:44 -0700 Comment on attachment 304447 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. 304447 2017-03-14 17:12:29 -0700 2017-03-14 17:15:28 -0700 proposed patch. bug-169647.patch text/plain 5338 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjEzOTYwKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE3LTAzLTE0ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBC eXRlY29kZUdlbmVyYXRvciBzaG91bGQgdXNlIHRoZSBzYW1lIGZ1bmN0aW9uIHRvIGRldGVybWlu ZSBpZiBpdCBuZWVkcyB0byBzdG9yZSB0aGUgRGVyaXZlZENvbnN0cnVjdG9yIGluIGFuIEFycm93 RnVuY3Rpb24gbGV4aWNhbCBlbnZpcm9ubWVudC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE2OTY0NworICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzEw NTE4MzI+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg KiBieXRlY29tcGlsZXIvQnl0ZWNvZGVHZW5lcmF0b3IuY3BwOgorICAgICAgICAoSlNDOjpCeXRl Y29kZUdlbmVyYXRvcjo6dXNlc0Rlcml2ZWRDb25zdHJ1Y3RvckluQXJyb3dGdW5jdGlvbkxleGlj YWxFbnZpcm9ubWVudCk6CisgICAgICAgIChKU0M6OkJ5dGVjb2RlR2VuZXJhdG9yOjppbml0aWFs aXplQXJyb3dGdW5jdGlvbkNvbnRleHRTY29wZUlmTmVlZGVkKToKKyAgICAgICAgKEpTQzo6Qnl0 ZWNvZGVHZW5lcmF0b3I6OmVtaXRQdXREZXJpdmVkQ29uc3RydWN0b3JUb0Fycm93RnVuY3Rpb25D b250ZXh0U2NvcGUpOgorICAgICAgICAqIGJ5dGVjb21waWxlci9CeXRlY29kZUdlbmVyYXRvci5o OgorCiAyMDE3LTAzLTE0ICBCcmlhbiBCdXJnICA8YmJ1cmdAYXBwbGUuY29tPgogCiAgICAgICAg IFtDb2NvYV0gV2ViIEluc3BlY3RvcjogZ2VuZXJhdGVkIGNvZGUgZm9yIHBhcnNpbmcgYW4gYXJy YXkgb2YgcHJpbWl0aXZlLXR5cGUgZW51bXMgZnJvbSBwYXlsb2FkIGRvZXMgbm90IHdvcmsKSW5k ZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvQnl0ZWNvZGVHZW5lcmF0b3Iu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvQnl0 ZWNvZGVHZW5lcmF0b3IuY3BwCShyZXZpc2lvbiAyMTM5MzEpCisrKyBTb3VyY2UvSmF2YVNjcmlw dENvcmUvYnl0ZWNvbXBpbGVyL0J5dGVjb2RlR2VuZXJhdG9yLmNwcAkod29ya2luZyBjb3B5KQpA QCAtMTAzMSw2ICsxMDMxLDE1IEBAIHZvaWQgQnl0ZWNvZGVHZW5lcmF0b3I6OmluaXRpYWxpemVE ZWZhdWwKICAgICB9CiB9CiAKK2Jvb2wgQnl0ZWNvZGVHZW5lcmF0b3I6Om5lZWRzRGVyaXZlZENv bnN0cnVjdG9ySW5BcnJvd0Z1bmN0aW9uTGV4aWNhbEVudmlyb25tZW50KCkKK3sKKyAgICBpZiAo KGlzQ29uc3RydWN0b3IoKSAmJiBjb25zdHJ1Y3RvcktpbmQoKSA9PSBDb25zdHJ1Y3RvcktpbmQ6 OkV4dGVuZHMpIHx8IG1fY29kZUJsb2NrLT5pc0NsYXNzQ29udGV4dCgpKSB7CisgICAgICAgIGlm IChpc1N1cGVyVXNlZEluSW5uZXJBcnJvd0Z1bmN0aW9uKCkpCisgICAgICAgICAgICByZXR1cm4g dHJ1ZTsKKyAgICB9CisgICAgcmV0dXJuIGZhbHNlOworfQorCiB2b2lkIEJ5dGVjb2RlR2VuZXJh dG9yOjppbml0aWFsaXplQXJyb3dGdW5jdGlvbkNvbnRleHRTY29wZUlmTmVlZGVkKFN5bWJvbFRh YmxlKiBmdW5jdGlvblN5bWJvbFRhYmxlLCBib29sIGNhblJldXNlTGV4aWNhbEVudmlyb25tZW50 KQogewogICAgIEFTU0VSVCghbV9hcnJvd0Z1bmN0aW9uQ29udGV4dExleGljYWxFbnZpcm9ubWVu dFJlZ2lzdGVyKTsKQEAgLTEwNTMsNyArMTA2Miw3IEBAIHZvaWQgQnl0ZWNvZGVHZW5lcmF0b3I6 OmluaXRpYWxpemVBcnJvd0YKICAgICAgICAgICAgIGZ1bmN0aW9uU3ltYm9sVGFibGUtPnNldChO b0xvY2tpbmdOZWNlc3NhcnksIHByb3BlcnR5TmFtZXMoKS5idWlsdGluTmFtZXMoKS5uZXdUYXJn ZXRMb2NhbFByaXZhdGVOYW1lKCkuaW1wbCgpLCBTeW1ib2xUYWJsZUVudHJ5KFZhck9mZnNldChv ZmZzZXQpKSk7CiAgICAgICAgIH0KICAgICAgICAgCi0gICAgICAgIGlmIChpc0NvbnN0cnVjdG9y KCkgJiYgY29uc3RydWN0b3JLaW5kKCkgPT0gQ29uc3RydWN0b3JLaW5kOjpFeHRlbmRzICYmIGlz U3VwZXJVc2VkSW5Jbm5lckFycm93RnVuY3Rpb24oKSkgeworICAgICAgICBpZiAobmVlZHNEZXJp dmVkQ29uc3RydWN0b3JJbkFycm93RnVuY3Rpb25MZXhpY2FsRW52aXJvbm1lbnQoKSkgewogICAg ICAgICAgICAgb2Zmc2V0ID0gZnVuY3Rpb25TeW1ib2xUYWJsZS0+dGFrZU5leHRTY29wZU9mZnNl dChOb0xvY2tpbmdOZWNlc3NhcnkpOwogICAgICAgICAgICAgZnVuY3Rpb25TeW1ib2xUYWJsZS0+ c2V0KE5vTG9ja2luZ05lY2Vzc2FyeSwgcHJvcGVydHlOYW1lcygpLmJ1aWx0aW5OYW1lcygpLmRl cml2ZWRDb25zdHJ1Y3RvclByaXZhdGVOYW1lKCkuaW1wbCgpLCBTeW1ib2xUYWJsZUVudHJ5KFZh ck9mZnNldChvZmZzZXQpKSk7CiAgICAgICAgIH0KQEAgLTEwNzUsNyArMTA4NCw3IEBAIHZvaWQg Qnl0ZWNvZGVHZW5lcmF0b3I6OmluaXRpYWxpemVBcnJvd0YKICAgICAgICAgYWRkVGFyZ2V0Lml0 ZXJhdG9yLT52YWx1ZS5zZXRJc0xldCgpOwogICAgIH0KIAotICAgIGlmIChpc0NvbnN0cnVjdG9y KCkgJiYgY29uc3RydWN0b3JLaW5kKCkgPT0gQ29uc3RydWN0b3JLaW5kOjpFeHRlbmRzICYmIGlz U3VwZXJVc2VkSW5Jbm5lckFycm93RnVuY3Rpb24oKSkgeworICAgIGlmIChuZWVkc0Rlcml2ZWRD b25zdHJ1Y3RvckluQXJyb3dGdW5jdGlvbkxleGljYWxFbnZpcm9ubWVudCgpKSB7CiAgICAgICAg IGF1dG8gZGVyaXZlZENvbnN0cnVjdG9yID0gZW52aXJvbm1lbnQuYWRkKHByb3BlcnR5TmFtZXMo KS5idWlsdGluTmFtZXMoKS5kZXJpdmVkQ29uc3RydWN0b3JQcml2YXRlTmFtZSgpKTsKICAgICAg ICAgZGVyaXZlZENvbnN0cnVjdG9yLml0ZXJhdG9yLT52YWx1ZS5zZXRJc0NhcHR1cmVkKCk7CiAg ICAgICAgIGRlcml2ZWRDb25zdHJ1Y3Rvci5pdGVyYXRvci0+dmFsdWUuc2V0SXNMZXQoKTsKQEAg LTQ1NzAsMTMgKzQ1NzksMTEgQEAgdm9pZCBCeXRlY29kZUdlbmVyYXRvcjo6ZW1pdFB1dE5ld1Rh cmdldAogICAgIAogdm9pZCBCeXRlY29kZUdlbmVyYXRvcjo6ZW1pdFB1dERlcml2ZWRDb25zdHJ1 Y3RvclRvQXJyb3dGdW5jdGlvbkNvbnRleHRTY29wZSgpCiB7Ci0gICAgaWYgKChpc0NvbnN0cnVj dG9yKCkgJiYgY29uc3RydWN0b3JLaW5kKCkgPT0gQ29uc3RydWN0b3JLaW5kOjpFeHRlbmRzKSB8 fCBtX2NvZGVCbG9jay0+aXNDbGFzc0NvbnRleHQoKSkgewotICAgICAgICBpZiAoaXNTdXBlclVz ZWRJbklubmVyQXJyb3dGdW5jdGlvbigpKSB7Ci0gICAgICAgICAgICBBU1NFUlQobV9hcnJvd0Z1 bmN0aW9uQ29udGV4dExleGljYWxFbnZpcm9ubWVudFJlZ2lzdGVyKTsKLSAgICAgICAgICAgIAot ICAgICAgICAgICAgVmFyaWFibGUgcHJvdG9TY29wZSA9IHZhcmlhYmxlKHByb3BlcnR5TmFtZXMo KS5idWlsdGluTmFtZXMoKS5kZXJpdmVkQ29uc3RydWN0b3JQcml2YXRlTmFtZSgpKTsKLSAgICAg ICAgICAgIGVtaXRQdXRUb1Njb3BlKG1fYXJyb3dGdW5jdGlvbkNvbnRleHRMZXhpY2FsRW52aXJv bm1lbnRSZWdpc3RlciwgcHJvdG9TY29wZSwgJm1fY2FsbGVlUmVnaXN0ZXIsIERvTm90VGhyb3dJ Zk5vdEZvdW5kLCBJbml0aWFsaXphdGlvbk1vZGU6OkluaXRpYWxpemF0aW9uKTsKLSAgICAgICAg fQorICAgIGlmIChuZWVkc0Rlcml2ZWRDb25zdHJ1Y3RvckluQXJyb3dGdW5jdGlvbkxleGljYWxF bnZpcm9ubWVudCgpKSB7CisgICAgICAgIEFTU0VSVChtX2Fycm93RnVuY3Rpb25Db250ZXh0TGV4 aWNhbEVudmlyb25tZW50UmVnaXN0ZXIpOworCisgICAgICAgIFZhcmlhYmxlIHByb3RvU2NvcGUg PSB2YXJpYWJsZShwcm9wZXJ0eU5hbWVzKCkuYnVpbHRpbk5hbWVzKCkuZGVyaXZlZENvbnN0cnVj dG9yUHJpdmF0ZU5hbWUoKSk7CisgICAgICAgIGVtaXRQdXRUb1Njb3BlKG1fYXJyb3dGdW5jdGlv bkNvbnRleHRMZXhpY2FsRW52aXJvbm1lbnRSZWdpc3RlciwgcHJvdG9TY29wZSwgJm1fY2FsbGVl UmVnaXN0ZXIsIERvTm90VGhyb3dJZk5vdEZvdW5kLCBJbml0aWFsaXphdGlvbk1vZGU6OkluaXRp YWxpemF0aW9uKTsKICAgICB9CiB9CiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRl Y29tcGlsZXIvQnl0ZWNvZGVHZW5lcmF0b3IuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNj cmlwdENvcmUvYnl0ZWNvbXBpbGVyL0J5dGVjb2RlR2VuZXJhdG9yLmgJKHJldmlzaW9uIDIxMzkz MSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvQnl0ZWNvZGVHZW5lcmF0 b3IuaAkod29ya2luZyBjb3B5KQpAQCAtMTAyNSw2ICsxMDI1LDcgQEAgbmFtZXNwYWNlIEpTQyB7 CiAgICAgICAgIHZvaWQgaW5pdGlhbGl6ZVZhckxleGljYWxFbnZpcm9ubWVudChpbnQgc3ltYm9s VGFibGVDb25zdGFudEluZGV4LCBTeW1ib2xUYWJsZSogZnVuY3Rpb25TeW1ib2xUYWJsZSwgYm9v bCBoYXNDYXB0dXJlZFZhcmlhYmxlcyk7CiAgICAgICAgIHZvaWQgaW5pdGlhbGl6ZURlZmF1bHRQ YXJhbWV0ZXJWYWx1ZXNBbmRTZXR1cEZ1bmN0aW9uU2NvcGVTdGFjayhGdW5jdGlvblBhcmFtZXRl cnMmLCBib29sIGlzU2ltcGxlUGFyYW1ldGVyTGlzdCwgRnVuY3Rpb25Ob2RlKiwgU3ltYm9sVGFi bGUqLCBpbnQgc3ltYm9sVGFibGVDb25zdGFudEluZGV4LCBjb25zdCBzdGQ6OmZ1bmN0aW9uPGJv b2wgKFVuaXF1ZWRTdHJpbmdJbXBsKik+JiBjYXB0dXJlcywgYm9vbCBzaG91bGRDcmVhdGVBcmd1 bWVudHNWYXJpYWJsZUluUGFyYW1ldGVyU2NvcGUpOwogICAgICAgICB2b2lkIGluaXRpYWxpemVB cnJvd0Z1bmN0aW9uQ29udGV4dFNjb3BlSWZOZWVkZWQoU3ltYm9sVGFibGUqIGZ1bmN0aW9uU3lt Ym9sVGFibGUgPSBudWxscHRyLCBib29sIGNhblJldXNlTGV4aWNhbEVudmlyb25tZW50ID0gZmFs c2UpOworICAgICAgICBib29sIG5lZWRzRGVyaXZlZENvbnN0cnVjdG9ySW5BcnJvd0Z1bmN0aW9u TGV4aWNhbEVudmlyb25tZW50KCk7CiAKICAgICBwdWJsaWM6CiAgICAgICAgIEpTU3RyaW5nKiBh ZGRTdHJpbmdDb25zdGFudChjb25zdCBJZGVudGlmaWVyJik7Cg==