180623 2017-12-09 10:04:27 -0800 Fix WTF::Hasher tuple expansion with variadic args 2017-12-10 09:58:17 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki achristensen benjamin cdumez cmarcelo commit-queue darin dbates ews-watchlist jfbastien mark.lam saam webkit-bug-importer oldest_to_newest 1379972 0 ysuzuki 2017-12-09 10:04:27 -0800 Fix WTF::Hasher tuple expansion with variadic args 1379974 1 328912 ysuzuki 2017-12-09 10:08:30 -0800 Created attachment 328912 Patch 1379987 2 328912 jfbastien 2017-12-09 12:05:54 -0800 Comment on attachment 328912 Patch r=me 1379991 3 328912 ysuzuki 2017-12-09 12:15:02 -0800 Comment on attachment 328912 Patch Thank you! 1379998 4 328912 commit-queue 2017-12-09 12:34:48 -0800 Comment on attachment 328912 Patch Clearing flags on attachment: 328912 Committed r225727: <https://trac.webkit.org/changeset/225727> 1379999 5 commit-queue 2017-12-09 12:34:50 -0800 All reviewed patches have been landed. Closing bug. 1380000 6 webkit-bug-importer 2017-12-09 12:35:27 -0800 <rdar://problem/35953223> 1380024 7 328912 darin 2017-12-09 16:20:39 -0800 Comment on attachment 328912 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328912&action=review > Source/WTF/ChangeLog:20 > + We expand a tuple with `...`, and use this in a function's argument list. > + And in this argument list, we call `add()` for each value. This will be > + expanded as follows. > + > + [] (...) { }((add(hasher, std::get<i>(values)), 0)...); > + > + will become, > + > + [] (...) { }((add(hasher, std::get<0>(values)), 0), (add(hasher, std::get<1>(values)), 0), ...); > + > + However, the evaluation order of the C++ argument is unspecified. Actually, > + in GCC environment, this `add()` is not called in our expected order. As a > + result, currently we have test failures in TestWTF. It’s strange, I got this from a webpage that talked about the issue with order of evaluation of function arguments and incorrectly claimed that something in this idiom made it correct and defined order! But now I can’t find that page. The new code you wrote looks good to me; I have very little experience with this relatively advanced C++ template meta programming. Apparently once we have C++17 we can use std::apply to do this more simply. 1380053 8 328912 ysuzuki 2017-12-10 04:58:46 -0800 Comment on attachment 328912 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328912&action=review >> Source/WTF/ChangeLog:20 >> + result, currently we have test failures in TestWTF. > > It’s strange, I got this from a webpage that talked about the issue with order of evaluation of function arguments and incorrectly claimed that something in this idiom made it correct and defined order! But now I can’t find that page. > > The new code you wrote looks good to me; I have very little experience with this relatively advanced C++ template meta programming. > > Apparently once we have C++17 we can use std::apply to do this more simply. I guess the intended way would be initializer list instead of function arguments, like, auto list = { (add(hasher, std::get<i>(values)), 0)... }; In that case, the evaluation order should be expected one since initialization order of this list is well-defined. 1380077 9 328912 darin 2017-12-10 09:58:17 -0800 Comment on attachment 328912 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328912&action=review >>> Source/WTF/ChangeLog:20 >>> + result, currently we have test failures in TestWTF. >> >> It’s strange, I got this from a webpage that talked about the issue with order of evaluation of function arguments and incorrectly claimed that something in this idiom made it correct and defined order! But now I can’t find that page. >> >> The new code you wrote looks good to me; I have very little experience with this relatively advanced C++ template meta programming. >> >> Apparently once we have C++17 we can use std::apply to do this more simply. > > I guess the intended way would be initializer list instead of function arguments, like, > > auto list = { > (add(hasher, std::get<i>(values)), 0)... > }; > > In that case, the evaluation order should be expected one since initialization order of this list is well-defined. Makes sense, but I literally copied the code from the webpage so it was definitely using a function call. Maybe I accidentally removed something. That will teach me to land something I don’t fully understand. I can be proud that I at least included good tests. 328912 2017-12-09 10:08:30 -0800 2017-12-09 12:34:48 -0800 Patch bug-180623-20171210030829.patch text/plain 4665 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjI1NzIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDQ2YzJhOWRhZTFjM2JjZWE2OTkyOTVl NGFkMGRlMjE0OGE3YWZkNWQuLjAwNjE5ZGRjYzUzZjFlYzljYzRkYmJiZDk1NTBjYWU3YzJiYzI0 N2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMzMgQEAKKzIwMTctMTItMDkgIFl1c3VrZSBTdXp1a2kgIDx1dGF0 YW5lLnRlYUBnbWFpbC5jb20+CisKKyAgICAgICAgRml4IFdURjo6SGFzaGVyIHR1cGxlIGV4cGFu c2lvbiB3aXRoIHZhcmlhZGljIGFyZ3MKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE4MDYyMworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlIGV4cGFuZCBhIHR1cGxlIHdpdGggYC4uLmAsIGFuZCB1c2UgdGhp cyBpbiBhIGZ1bmN0aW9uJ3MgYXJndW1lbnQgbGlzdC4KKyAgICAgICAgQW5kIGluIHRoaXMgYXJn dW1lbnQgbGlzdCwgd2UgY2FsbCBgYWRkKClgIGZvciBlYWNoIHZhbHVlLiBUaGlzIHdpbGwgYmUK KyAgICAgICAgZXhwYW5kZWQgYXMgZm9sbG93cy4KKworICAgICAgICAgICAgW10gKC4uLikgeyB9 KChhZGQoaGFzaGVyLCBzdGQ6OmdldDxpPih2YWx1ZXMpKSwgMCkuLi4pOworCisgICAgICAgIHdp bGwgYmVjb21lLAorCisgICAgICAgICAgICBbXSAoLi4uKSB7IH0oKGFkZChoYXNoZXIsIHN0ZDo6 Z2V0PDA+KHZhbHVlcykpLCAwKSwgKGFkZChoYXNoZXIsIHN0ZDo6Z2V0PDE+KHZhbHVlcykpLCAw KSwgLi4uKTsKKworICAgICAgICBIb3dldmVyLCB0aGUgZXZhbHVhdGlvbiBvcmRlciBvZiB0aGUg QysrIGFyZ3VtZW50IGlzIHVuc3BlY2lmaWVkLiBBY3R1YWxseSwKKyAgICAgICAgaW4gR0NDIGVu dmlyb25tZW50LCB0aGlzIGBhZGQoKWAgaXMgbm90IGNhbGxlZCBpbiBvdXIgZXhwZWN0ZWQgb3Jk ZXIuIEFzIGEKKyAgICAgICAgcmVzdWx0LCBjdXJyZW50bHkgd2UgaGF2ZSB0ZXN0IGZhaWx1cmVz IGluIFRlc3RXVEYuCisKKyAgICAgICAgVGhpcyBwYXRjaCBqdXN0IHVzZXMgdmFyaWFkaWMgdGVt cGxhdGVzIHRvIGV4cGFuZCB0dXBsZXMsIGFuZCBjYWxsIGFkZCgpIGluCisgICAgICAgIG91ciBl eHBlY3RlZCBvcmRlci4gVGhpcyBwYXRjaCBmaXhlcyBhbiBleGlzdGluZyBmYWlsdXJlIG9mIFRl c3RXVEYgaW4gR0NDIGVudmlyb25tZW50LgorCisgICAgICAgICogd3RmL0hhc2hlci5oOgorICAg ICAgICAoV1RGOjpIYXNoZXI6OmNvbXB1dGVIYXNoKToKKyAgICAgICAgKFdURjo6YWRkQXJncyk6 CisgICAgICAgIChXVEY6OmFkZFR1cGxlSGVscGVyKToKKyAgICAgICAgKFdURjo6YWRkKToKKwog MjAxNy0xMi0wOCAgRXJpYyBDYXJsc29uICA8ZXJpYy5jYXJsc29uQGFwcGxlLmNvbT4KIAogICAg ICAgICBNb3ZlIExvZ2dlciBmcm9tIFBBTCB0byBXVEYgc28gaXQgY2FuIGJlIHVzZWQgb3V0c2lk ZSBvZiBXZWJDb3JlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9IYXNoZXIuaCBiL1NvdXJj ZS9XVEYvd3RmL0hhc2hlci5oCmluZGV4IDhhMDk0M2VlY2NhZDQyY2ZiZGE2NjkxNDc2OWM3YmUx YzZhZTBkMjcuLjE0MzkzOTFjZDUyMDIwYThjZTk4YjI4MzExZTM0MmRkOGUzMDk4Y2MgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0hhc2hlci5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0hhc2hl ci5oCkBAIC01MSw3ICs1MSw3IEBAIGNsYXNzIEhhc2hlciB7CiAgICAgdGVtcGxhdGU8dHlwZW5h bWUuLi4gVHlwZXM+IGZyaWVuZCB1aW50MzJfdCBjb21wdXRlSGFzaChjb25zdCBUeXBlcyYuLi4g dmFsdWVzKQogICAgIHsKICAgICAgICAgSGFzaGVyIGhhc2hlcjsKLSAgICAgICAgYWRkKGhhc2hl ciwgc3RkOjpmb3J3YXJkX2FzX3R1cGxlKHZhbHVlcy4uLikpOworICAgICAgICBhZGRBcmdzKGhh c2hlciwgdmFsdWVzLi4uKTsKICAgICAgICAgcmV0dXJuIGhhc2hlci5tX3VuZGVybHlpbmdIYXNo ZXIuaGFzaCgpOwogICAgIH0KIApAQCAtNTksNyArNTksNyBAQCBjbGFzcyBIYXNoZXIgewogICAg IHsKICAgICAgICAgSGFzaGVyIGhhc2hlcjsKICAgICAgICAgYWRkKGhhc2hlciwgbGlzdCk7Ci0g ICAgICAgIGFkZChoYXNoZXIsIHN0ZDo6Zm9yd2FyZF9hc190dXBsZShvdGhlckxpc3RzLi4uKSk7 CisgICAgICAgIGFkZEFyZ3MoaGFzaGVyLCBvdGhlckxpc3RzLi4uKTsKICAgICAgICAgcmV0dXJu IGhhc2hlci5tX3VuZGVybHlpbmdIYXNoZXIuaGFzaCgpOwogICAgIH0KIApAQCAtMTEzLDkgKzEx MywxOSBAQCB0ZW1wbGF0ZTx0eXBlbmFtZSBDb250YWluZXI+IHN0ZDo6ZW5hYmxlX2lmX3Q8SGFz QmVnaW5GdW5jdGlvbk1lbWJlcjxDb250YWluZXI+OgogICAgICAgICBhZGQoaGFzaGVyLCB2YWx1 ZSk7CiB9CiAKK2lubGluZSB2b2lkIGFkZEFyZ3MoSGFzaGVyJikKK3sKK30KKwordGVtcGxhdGU8 dHlwZW5hbWUgQXJnLCB0eXBlbmFtZSAuLi5BcmdzPiB2b2lkIGFkZEFyZ3MoSGFzaGVyJiBoYXNo ZXIsIGNvbnN0IEFyZyYgYXJnLCBjb25zdCBBcmdzJi4uLiBhcmdzKQoreworICAgIGFkZChoYXNo ZXIsIGFyZyk7CisgICAgYWRkQXJncyhoYXNoZXIsIGFyZ3MuLi4pOworfQorCiB0ZW1wbGF0ZTx0 eXBlbmFtZSBUdXBsZSwgc3RkOjpzaXplX3QgLi4uaT4gdm9pZCBhZGRUdXBsZUhlbHBlcihIYXNo ZXImIGhhc2hlciwgY29uc3QgVHVwbGUmIHZhbHVlcywgc3RkOjppbmRleF9zZXF1ZW5jZTxpLi4u PikKIHsKLSAgICBbXSAoLi4uKSB7IH0oKGFkZChoYXNoZXIsIHN0ZDo6Z2V0PGk+KHZhbHVlcykp LCAwKS4uLik7CisgICAgYWRkQXJncyhoYXNoZXIsIHN0ZDo6Z2V0PGk+KHZhbHVlcykuLi4pOwog fQogCiB0ZW1wbGF0ZTx0eXBlbmFtZS4uLiBUeXBlcz4gdm9pZCBhZGQoSGFzaGVyJiBoYXNoZXIs IGNvbnN0IHN0ZDo6dHVwbGU8VHlwZXMuLi4+JiB0dXBsZSkKQEAgLTE0OCw3ICsxNTgsNyBAQCB0 ZW1wbGF0ZTx0eXBlbmFtZSBUMSwgdHlwZW5hbWUgVDIsIHR5cGVuYW1lLi4uIE90aGVyVHlwZXM+ IHZvaWQgYWRkKEhhc2hlciYgaGFzaAogewogICAgIGFkZChoYXNoZXIsIHZhbHVlMSk7CiAgICAg YWRkKGhhc2hlciwgdmFsdWUyKTsKLSAgICBhZGQoaGFzaGVyLCBzdGQ6OmZvcndhcmRfYXNfdHVw bGUob3RoZXJWYWx1ZXMuLi4pKTsKKyAgICBhZGRBcmdzKGhhc2hlciwgb3RoZXJWYWx1ZXMuLi4p OwogfQogCiB0ZW1wbGF0ZTx0eXBlbmFtZSBUPiB2b2lkIGFkZChIYXNoZXImIGhhc2hlciwgc3Rk Ojppbml0aWFsaXplcl9saXN0PFQ+IHZhbHVlcykKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxv ZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCA2MmY2NjNlZmVmODBjNzQyM2RjNzljN2FjM2Q1ZWRk NTM3NmY5NTg3Li4xMDQ5NWFjYWQwMDY0YTUwYzkxMjJiNTlmZDI2OGY0YjMxNzM0OGNjIDEwMDY0 NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEs MTMgQEAKKzIwMTctMTItMDkgIFl1c3VrZSBTdXp1a2kgIDx1dGF0YW5lLnRlYUBnbWFpbC5jb20+ CisKKyAgICAgICAgRml4IFdURjo6SGFzaGVyIHR1cGxlIGV4cGFuc2lvbiB3aXRoIHZhcmlhZGlj IGFyZ3MKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 MDYyMworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog VGVzdFdlYktpdEFQSS9UZXN0cy9XVEYvSGFzaGVyLmNwcDoKKyAgICAgICAgKFRlc3RXZWJLaXRB UEk6OlRFU1QpOgorCiAyMDE3LTEyLTA4ICBCYXN1a2UgU3V6dWtpICA8QmFzdWtlLlN1enVraUBz b255LmNvbT4KIAogICAgICAgICBbV2luXSBUaGUgd2F5IHRvIGRldGVjdCBXaW5kb3dzIDEwIGlz IHdyb25nCmRpZmYgLS1naXQgYS9Ub29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9IYXNoZXIu Y3BwIGIvVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0cy9XVEYvSGFzaGVyLmNwcAppbmRleCA3NDY2 ZTA3MTNhYjkyYWM2YjVjZWJhNzE4Njk4ZjJiN2I1ZWExMzI5Li41ZDdiNDAxOTkxY2QxMGJjN2My MzlhZTc4ZjQ1Njc2YTNkZDkyYjNjIDEwMDY0NAotLS0gYS9Ub29scy9UZXN0V2ViS2l0QVBJL1Rl c3RzL1dURi9IYXNoZXIuY3BwCisrKyBiL1Rvb2xzL1Rlc3RXZWJLaXRBUEkvVGVzdHMvV1RGL0hh c2hlci5jcHAKQEAgLTE2OSw2ICsxNjksNyBAQCBURVNUKFdURiwgSGFzaGVyX211bHRpcGxlKQog ICAgIEVYUEVDVF9FUSh6ZXJvMzJCaXRIYXNoLCBjb21wdXRlSGFzaChWZWN0b3I8aW50PiB7IDAg fSkpOwogICAgIEVYUEVDVF9FUSh6ZXJvMzJCaXRIYXNoLCBjb21wdXRlSGFzaChWZWN0b3I8aW50 LCAxPiB7IDAgfSkpOwogICAgIEVYUEVDVF9FUSh6ZXJvMzJCaXRIYXNoLCBjb21wdXRlSGFzaChz dGQ6Om9wdGlvbmFsPGludD4geyBzdGQ6Om51bGxvcHQgfSkpOworICAgIEVYUEVDVF9FUSh6ZXJv MzJCaXRIYXNoLCBjb21wdXRlSGFzaChzdGQ6Om1ha2VfdHVwbGUoMCkpKTsKIAogICAgIEVYUEVD VF9FUShvbmU2NEJpdEhhc2gsIGNvbXB1dGVIYXNoKDEsIDApKTsKICAgICBFWFBFQ1RfRVEob25l NjRCaXRIYXNoLCBjb21wdXRlSGFzaChzdGQ6Om1ha2VfdHVwbGUoMSwgMCkpKTsK