Bug 180623 – Fix WTF::Hasher tuple expansion with variadic args

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Fix WTF::Hasher tuple expansion with variadic args

[Yusuke Suzuki]

Created attachment 328912 [details]
Patch

[JF Bastien]

Comment on attachment 328912 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 328912 [details]
Patch

Thank you!

[WebKit Commit Bot]

Comment on attachment 328912 [details]
Patch

Clearing flags on attachment: 328912

Committed r225727: <https://trac.webkit.org/changeset/225727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35953223>

[Darin Adler]

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.

[Yusuke Suzuki]

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.

[Darin Adler]

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.