Bug 180623 - Fix WTF::Hasher tuple expansion with variadic args
Summary: Fix WTF::Hasher tuple expansion with variadic args
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-09 10:04 PST by Yusuke Suzuki
Modified: 2017-12-10 09:58 PST (History)
12 users (show)

See Also:


Attachments
Patch (4.56 KB, patch)
2017-12-09 10:08 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-12-09 10:04:27 PST
Fix WTF::Hasher tuple expansion with variadic args
Comment 1 Yusuke Suzuki 2017-12-09 10:08:30 PST
Created attachment 328912 [details]
Patch
Comment 2 JF Bastien 2017-12-09 12:05:54 PST
Comment on attachment 328912 [details]
Patch

r=me
Comment 3 Yusuke Suzuki 2017-12-09 12:15:02 PST
Comment on attachment 328912 [details]
Patch

Thank you!
Comment 4 WebKit Commit Bot 2017-12-09 12:34:48 PST
Comment on attachment 328912 [details]
Patch

Clearing flags on attachment: 328912

Committed r225727: <https://trac.webkit.org/changeset/225727>
Comment 5 WebKit Commit Bot 2017-12-09 12:34:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2017-12-09 12:35:27 PST
<rdar://problem/35953223>
Comment 7 Darin Adler 2017-12-09 16:20:39 PST
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 8 Yusuke Suzuki 2017-12-10 04:58:46 PST
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 9 Darin Adler 2017-12-10 09:58:17 PST
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.