Fix WTF::Hasher tuple expansion with variadic args
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.
<rdar://problem/35953223>
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.