Bug 158542 - Greatly simplify CrossThreadTask.h
Summary: Greatly simplify CrossThreadTask.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-08 14:20 PDT by Brady Eidson
Modified: 2016-06-09 10:41 PDT (History)
7 users (show)

See Also:

Attachments
Patch (14.49 KB, patch)
2016-06-08 14:22 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (14.74 KB, patch)
2016-06-08 14:51 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (14.95 KB, patch)
2016-06-08 15:40 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Patch (14.63 KB, patch)
2016-06-08 15:57 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-06-08 14:20:23 PDT 
Greatly simplify CrossThreadTask.h

The current implementation predates variadic templates in C++11.

Now with C++14 we can get rid of most of this nonsense.

There is an additional move constructor call per argument object, which is disappointing.

Maybe there's a way to get rid of that and I can't see that way.

But compared to two extra copies (which is what we needed up through this morning before I landed http://trac.webkit.org/changeset/201809) we're still way better off.
Comment 1 Brady Eidson 2016-06-08 14:22:35 PDT 
Created attachment 280837 [details]
Patch
Comment 2 Sam Weinig 2016-06-08 14:31:49 PDT 
Can this use std::index_sequence rather than the homespun GeneratedIndices? For an example usage of it, see WebKit2’s HandleMessage.h.
Comment 3 Brady Eidson 2016-06-08 14:32:43 PDT 
(In reply to comment #2)
> Can this use std::index_sequence rather than the homespun GeneratedIndices?
> For an example usage of it, see WebKit2’s HandleMessage.h.

Holy crap, yes.
Comment 4 Brady Eidson 2016-06-08 14:51:13 PDT 
Created attachment 280843 [details]
Patch
Comment 5 Brady Eidson 2016-06-08 15:19:59 PDT 
Yikes on the build failures.

Currently exploring why I'm seemingly good locally...
Comment 6 Brady Eidson 2016-06-08 15:40:04 PDT 
Created attachment 280849 [details]
Patch
Comment 7 Brady Eidson 2016-06-08 15:57:17 PDT 
Created attachment 280852 [details]
Patch
Comment 8 Saam Barati 2016-06-09 00:50:58 PDT 
Comment on attachment 280852 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280852&action=review

> Source/WTF/wtf/CrossThreadTask.h:80
> +void callMemberFunctionForCrossThreadTaskImpl(C* object, MF function, ArgsTuple&& args, std::index_sequence<ArgsIndex...>)

std::index_sequence is quite clever.
After enough reading on the web, I actually understand what's happening here.
Comment 9 Darin Adler 2016-06-09 10:10:31 PDT 
Comment on attachment 280852 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280852&action=review

> Source/WTF/wtf/CrossThreadTask.h:65
> +template <typename F, typename ArgsTuple, typename ArgsIndicies = std::make_index_sequence<std::tuple_size<ArgsTuple>::value>>

The word indices has only two "i"s in it.
Comment 10 WebKit Commit Bot 2016-06-09 10:33:25 PDT 
Comment on attachment 280852 [details]
Patch

Clearing flags on attachment: 280852

Committed r201872: <http://trac.webkit.org/changeset/201872>
Comment 11 WebKit Commit Bot 2016-06-09 10:33:30 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 Brady Eidson 2016-06-09 10:41:28 PDT 
(In reply to comment #10)
> Comment on attachment 280852 [details]
> Patch
> 
> Clearing flags on attachment: 280852
> 
> Committed r201872: <http://trac.webkit.org/changeset/201872>

Fixed the typo in http://trac.webkit.org/changeset/201873