177892 2017-10-04 12:55:06 -0700 JSC: use default_construct_at 2018-03-28 23:40:50 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 177473 1 jfbastien jfbastien ap dbates fpizlo jfbastien keith_miller mark.lam msaboff rmorisset saam oldest_to_newest 1356600 0 jfbastien 2017-10-04 12:55:06 -0700 I added default_construct_at to StdLibExtra in #177473 because some code was otherwise hard to read. I wanted to see how it would feel in other parts of the code, and whether it was a widespread thing that we'd use, so I grep'd for a few places in JSC where I could use it, and did so! I'm thinking about proposing this for C++20, so getting a feel for it seems useful, and in this case causes very little churn. 1356601 1 322707 jfbastien 2017-10-04 12:56:43 -0700 Created attachment 322707 patch Does it look nicer? More understandable? 1356603 2 jfbastien 2017-10-04 12:57:36 -0700 Forgot to say: an easy way to grep for likely uses is: git grep "new (" ./Source/JavaScriptCore/ | grep -v NotNull | grep "()" 1356606 3 saam 2017-10-04 13:01:50 -0700 I vote for default_construct_at => construct_at construct_at should just take variadic parameters it does perfect forwarding on. "Default" flows naturally out of that, e.g, no arguments passed. 1356708 4 322707 ap 2017-10-04 16:29:59 -0700 Comment on attachment 322707 patch View in context: https://bugs.webkit.org/attachment.cgi?id=322707&action=review > Source/JavaScriptCore/ChangeLog:8 > + It makes code cleaner. I know how to read placement new, but I would definitely need to look up what default_construct_at or construct_at were. In general, it seems undesirable to use 1:1 replacements for language constructs, as that adds barriers to entry. > Source/JavaScriptCore/yarr/YarrInterpreter.cpp:91 > - return new (allocatorPool->alloc(size)) DisjunctionContext(); > + return &default_construct_at(static_cast<DisjunctionContext*>(allocatorPool->alloc(size))); This in particular seems harder to read than before. 1356714 5 saam 2017-10-04 16:38:14 -0700 (In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 322707 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322707&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + It makes code cleaner. > > I know how to read placement new, but I would definitely need to look up > what default_construct_at or construct_at were. > > In general, it seems undesirable to use 1:1 replacements for language > constructs, as that adds barriers to entry. > > > Source/JavaScriptCore/yarr/YarrInterpreter.cpp:91 > > - return new (allocatorPool->alloc(size)) DisjunctionContext(); > > + return &default_construct_at(static_cast<DisjunctionContext*>(allocatorPool->alloc(size))); > > This in particular seems harder to read than before. I agree. I much prefer placement new. 1362549 6 jfbastien 2017-10-19 21:22:27 -0700 Agreed with Saam's suggestion, can forward things. Code such as this is definitely harder to write as well as read: new (&container.get()[i]) std::remove_reference_t<decltype(*container.get())>(); That's the original inspiration for the initial proposal. That being said it's one place, so I don't really care. 1410196 7 322707 dbates 2018-03-28 23:40:50 -0700 Comment on attachment 322707 patch Clearing r and cq flags to get this patch out of these queues. 322707 2017-10-04 12:56:43 -0700 2018-03-28 23:40:50 -0700 patch 0001-JSC-use-default_construct_at.patch text/plain 4549 jfbastien RnJvbSBiMDBiZGIyYzhiNzJiNDY1MWMxM2FjOTE4ZjI2MTMxMzg4ODU2MmZjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKRiBCYXN0aWVuIDxqZmJhc3RpZW5AYXBwbGUuY29tPgpEYXRl OiBXZWQsIDQgT2N0IDIwMTcgMTI6NTU6NTUgLTA3MDAKU3ViamVjdDogW1BBVENIXSBKU0M6IHVz ZSBkZWZhdWx0X2NvbnN0cnVjdF9hdAoKLS0tCiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nICAgICAgICAgICAgICAgICAgICAgfCAxNyArKysrKysrKysrKysrKysrKwogU291cmNlL0ph dmFTY3JpcHRDb3JlL2J5dGVjb2RlL1ZhbHVlUHJvZmlsZS5oICAgICAgIHwgIDUgKysrLS0KIFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0hhc093blByb3BlcnR5Q2FjaGUuaCB8ICAzICsr LQogU291cmNlL0phdmFTY3JpcHRDb3JlL3lhcnIvWWFyckludGVycHJldGVyLmNwcCAgICAgIHwg IDUgKysrLS0KIDQgZmlsZXMgY2hhbmdlZCwgMjUgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMo LSkKCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nIGIvU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAzNGQ3YzMyLi4xODAwMDAwIDEwMDY0NAot LS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxNy0xMC0wNCAgSkYgQmFzdGll biAgPGpmYmFzdGllbkBhcHBsZS5jb20+CisKKyAgICAgICAgSlNDOiB1c2UgZGVmYXVsdF9jb25z dHJ1Y3RfYXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE3Nzg5MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEl0IG1ha2VzIGNvZGUgY2xlYW5lci4KKworICAgICAgICAqIGJ5dGVjb2RlL1ZhbHVlUHJvZmls ZS5oOgorICAgICAgICAoSlNDOjpWYWx1ZVByb2ZpbGVBbmRPcGVyYW5kQnVmZmVyOjpWYWx1ZVBy b2ZpbGVBbmRPcGVyYW5kQnVmZmVyKToKKyAgICAgICAgKiBydW50aW1lL0hhc093blByb3BlcnR5 Q2FjaGUuaDoKKyAgICAgICAgKEpTQzo6SGFzT3duUHJvcGVydHlDYWNoZTo6Y2xlYXJCdWZmZXIp OgorICAgICAgICAqIHlhcnIvWWFyckludGVycHJldGVyLmNwcDoKKyAgICAgICAgKEpTQzo6WWFy cjo6SW50ZXJwcmV0ZXI6OmFsbG9jRGlzanVuY3Rpb25Db250ZXh0KToKKyAgICAgICAgKEpTQzo6 WWFycjo6SW50ZXJwcmV0ZXI6OlBhcmVudGhlc2VzRGlzanVuY3Rpb25Db250ZXh0OjpQYXJlbnRo ZXNlc0Rpc2p1bmN0aW9uQ29udGV4dCk6CisKIDIwMTctMTAtMDQgIENhcmxvcyBBbGJlcnRvIExv cGV6IFBlcmV6ICA8Y2xvcGV6QGlnYWxpYS5jb20+CiAKICAgICAgICAgR2VuZXJhdGUgYSBjb21w aWxlIGVycm9yIGlmIHJlbGVhc2UgaXMgYnVpbHQgd2l0aG91dCBjb21waWxlciBvcHRpbWl6YXRp b25zCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvVmFsdWVQcm9m aWxlLmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvZGUvVmFsdWVQcm9maWxlLmgKaW5k ZXggYzgxNDMwMC4uNjNiNDBlNiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2J5 dGVjb2RlL1ZhbHVlUHJvZmlsZS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29k ZS9WYWx1ZVByb2ZpbGUuaApAQCAtMSw1ICsxLDUgQEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIw MTEtMjAxMywgMjAxNiBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCisgKiBDb3B5cmln aHQgKEMpIDIwMTEtMjAxNyBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCiAgKgogICog UmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5kIGJpbmFyeSBmb3Jtcywgd2l0aCBv ciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24sIGFyZSBwZXJtaXR0ZWQgcHJvdmlkZWQgdGhhdCB0 aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAgLTMyLDYgKzMyLDcgQEAKICNpbmNsdWRlICJTcGVj dWxhdGVkVHlwZS5oIgogI2luY2x1ZGUgIlN0cnVjdHVyZS5oIgogI2luY2x1ZGUgPHd0Zi9Qcmlu dFN0cmVhbS5oPgorI2luY2x1ZGUgPHd0Zi9TdGRMaWJFeHRyYXMuaD4KICNpbmNsdWRlIDx3dGYv U3RyaW5nUHJpbnRTdHJlYW0uaD4KIAogbmFtZXNwYWNlIEpTQyB7CkBAIC0yMTYsNyArMjE3LDcg QEAgc3RydWN0IFZhbHVlUHJvZmlsZUFuZE9wZXJhbmRCdWZmZXIgewogICAgICAgICAvLyBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc1NDEzCiAgICAgICAgIG1fYnVm ZmVyID0gTWFsbG9jUHRyPFZhbHVlUHJvZmlsZUFuZE9wZXJhbmQ+OjptYWxsb2MobV9zaXplICog c2l6ZW9mKFZhbHVlUHJvZmlsZUFuZE9wZXJhbmQpKTsKICAgICAgICAgZm9yICh1bnNpZ25lZCBp ID0gMDsgaSA8IG1fc2l6ZTsgKytpKQotICAgICAgICAgICAgbmV3ICgmbV9idWZmZXIuZ2V0KClb aV0pIFZhbHVlUHJvZmlsZUFuZE9wZXJhbmQoKTsKKyAgICAgICAgICAgIGRlZmF1bHRfY29uc3Ry dWN0X2F0KCZtX2J1ZmZlci5nZXQoKVtpXSk7CiAgICAgfQogCiAgICAgflZhbHVlUHJvZmlsZUFu ZE9wZXJhbmRCdWZmZXIoKQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRp bWUvSGFzT3duUHJvcGVydHlDYWNoZS5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUv SGFzT3duUHJvcGVydHlDYWNoZS5oCmluZGV4IDc2Mjc1MTYuLjY3MTg4NzYgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0hhc093blByb3BlcnR5Q2FjaGUuaAorKysg Yi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9IYXNPd25Qcm9wZXJ0eUNhY2hlLmgKQEAg LTI4LDYgKzI4LDcgQEAKICNpbmNsdWRlICJKU09iamVjdC5oIgogI2luY2x1ZGUgIlByb3BlcnR5 U2xvdC5oIgogI2luY2x1ZGUgIlN0cnVjdHVyZS5oIgorI2luY2x1ZGUgPHd0Zi9TdGRMaWJFeHRy YXMuaD4KIAogbmFtZXNwYWNlIEpTQyB7CiAKQEAgLTEzNSw3ICsxMzYsNyBAQCBwcml2YXRlOgog ICAgIHsKICAgICAgICAgRW50cnkqIGJ1ZmZlciA9IGJpdHdpc2VfY2FzdDxFbnRyeSo+KHRoaXMp OwogICAgICAgICBmb3IgKHVpbnQzMl90IGkgPSAwOyBpIDwgc2l6ZTsgKytpKQotICAgICAgICAg ICAgbmV3ICgmYnVmZmVyW2ldKSBFbnRyeSgpOworICAgICAgICAgICAgZGVmYXVsdF9jb25zdHJ1 Y3RfYXQoJmJ1ZmZlcltpXSk7CiAgICAgfQogfTsKIApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL3lhcnIvWWFyckludGVycHJldGVyLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS95YXJyL1lhcnJJbnRlcnByZXRlci5jcHAKaW5kZXggNDhjNGE4ZmYuLjhjMDJlMjcgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS95YXJyL1lhcnJJbnRlcnByZXRlci5jcHAKKysr IGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3lhcnIvWWFyckludGVycHJldGVyLmNwcApAQCAtMzIs NiArMzIsNyBAQAogI2luY2x1ZGUgIllhcnJDYW5vbmljYWxpemUuaCIKICNpbmNsdWRlIDx3dGYv QnVtcFBvaW50ZXJBbGxvY2F0b3IuaD4KICNpbmNsdWRlIDx3dGYvRGF0YUxvZy5oPgorI2luY2x1 ZGUgPHd0Zi9TdGRMaWJFeHRyYXMuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+CiAj aW5jbHVkZSA8d3RmL3RleHQvV1RGU3RyaW5nLmg+CiAKQEAgLTg3LDcgKzg4LDcgQEAgcHVibGlj OgogICAgICAgICBzaXplX3Qgc2l6ZSA9IHNpemVvZihEaXNqdW5jdGlvbkNvbnRleHQpIC0gc2l6 ZW9mKHVpbnRwdHJfdCkgKyBkaXNqdW5jdGlvbi0+bV9mcmFtZVNpemUgKiBzaXplb2YodWludHB0 cl90KTsKICAgICAgICAgYWxsb2NhdG9yUG9vbCA9IGFsbG9jYXRvclBvb2wtPmVuc3VyZUNhcGFj aXR5KHNpemUpOwogICAgICAgICBSRUxFQVNFX0FTU0VSVChhbGxvY2F0b3JQb29sKTsKLSAgICAg ICAgcmV0dXJuIG5ldyAoYWxsb2NhdG9yUG9vbC0+YWxsb2Moc2l6ZSkpIERpc2p1bmN0aW9uQ29u dGV4dCgpOworICAgICAgICByZXR1cm4gJmRlZmF1bHRfY29uc3RydWN0X2F0KHN0YXRpY19jYXN0 PERpc2p1bmN0aW9uQ29udGV4dCo+KGFsbG9jYXRvclBvb2wtPmFsbG9jKHNpemUpKSk7CiAgICAg fQogCiAgICAgdm9pZCBmcmVlRGlzanVuY3Rpb25Db250ZXh0KERpc2p1bmN0aW9uQ29udGV4dCog Y29udGV4dCkKQEAgLTEwOCw3ICsxMDksNyBAQCBwdWJsaWM6CiAgICAgICAgICAgICAgICAgb3V0 cHV0WyhmaXJzdFN1YnBhdHRlcm5JZCA8PCAxKSArIGldID0gb2Zmc2V0Tm9NYXRjaDsKICAgICAg ICAgICAgIH0KIAotICAgICAgICAgICAgbmV3IChnZXREaXNqdW5jdGlvbkNvbnRleHQodGVybSkp IERpc2p1bmN0aW9uQ29udGV4dCgpOworICAgICAgICAgICAgZGVmYXVsdF9jb25zdHJ1Y3RfYXQo Z2V0RGlzanVuY3Rpb25Db250ZXh0KHRlcm0pKTsKICAgICAgICAgfQogCiAgICAgICAgIHZvaWQq IG9wZXJhdG9yIG5ldyhzaXplX3QsIHZvaWQqIHdoZXJlKQotLSAKMi45LjMKCg==