Summary: | Fix WTF::Hasher tuple expansion with variadic args | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, jfbastien, mark.lam, saam, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-12-09 10:04:27 PST
Created attachment 328912 [details]
Patch
Comment on attachment 328912 [details]
Patch
r=me
Comment on attachment 328912 [details]
Patch
Thank you!
Comment on attachment 328912 [details] Patch Clearing flags on attachment: 328912 Committed r225727: <https://trac.webkit.org/changeset/225727> All reviewed patches have been landed. Closing bug. Comment on attachment 328912 [details] 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. Comment on attachment 328912 [details] 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. Comment on attachment 328912 [details] 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. |