Bug 137000

Summary: [WinCairo] Compile errors when GStreamer is enabled.
Product: WebKit Reporter: peavo
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.christensen, benjamin, bfulgham, cmarcelo, commit-queue, ossy, pnormand, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description peavo 2014-09-22 06:51:56 PDT
MSVC does not allow the keyword default on move constructors. Also, MSVC cannot assign an initializer list to a struct.
Comment 1 peavo 2014-09-22 06:57:45 PDT
Created attachment 238479 [details]
Patch
Comment 2 Philippe Normand 2014-09-23 01:09:23 PDT
This breaks GTK and EFL though. Can you please fix it?
Comment 3 Zan Dobersek 2014-09-23 01:14:09 PDT
What version of MSVC?

(In reply to comment #0)
> Also, MSVC cannot assign an initializer list to a struct.

MSVC 2013 supposedly supports this. Does it support initializing a class object via the initializer list?
Comment 4 Zan Dobersek 2014-09-23 01:16:17 PDT
Comment on attachment 238479 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.h:98
> +            *this = c;

That's a copy. Use WTF::move().
Comment 5 Zan Dobersek 2014-09-23 02:07:27 PDT
(In reply to comment #3)
> What version of MSVC?
> 
> (In reply to comment #0)
> > Also, MSVC cannot assign an initializer list to a struct.
> 
> MSVC 2013 supposedly supports this. Does it support initializing a class object via the initializer list?

For clarity, here's the MSDN document:
http://msdn.microsoft.com/en-us/library/dn387583.aspx

Does MSVC compile this?

    m_context = Context({
        adoptGRef(g_socket_create_source(socket, condition, socketCancellable)),
        adoptGRef(g_cancellable_new()),
        adoptGRef(socketCancellable),
        nullptr, // voidCallback
        nullptr, // boolCallback
        WTF::move(function),
        WTF::move(destroyFunction)
    });
Comment 6 peavo 2014-09-23 07:16:42 PDT
(In reply to comment #5)
> 
> Does MSVC compile this?
> 
>     m_context = Context({
>         adoptGRef(g_socket_create_source(socket, condition, socketCancellable)),
>         adoptGRef(g_cancellable_new()),
>         adoptGRef(socketCancellable),
>         nullptr, // voidCallback
>         nullptr, // boolCallback
>         WTF::move(function),
>         WTF::move(destroyFunction)
>     });

No, the error is:

1>..\wtf\gobject\GMainLoopSource.cpp(130): error C2440: 'initializing' : cannot convert from 'initializer-list' to 'WTF::GMainLoopSource::Context'
1>          No constructor could take the source type, or constructor overload resolution was ambiguous
Comment 7 peavo 2014-09-23 07:25:07 PDT
Created attachment 238537 [details]
Patch
Comment 8 peavo 2014-09-23 07:30:21 PDT
(In reply to comment #3)
> What version of MSVC?
> 

MSVC 2013.

The original error was:

..\wtf\gobject\GMainLoopSource.cpp(130): error C2679: binary '=' : no operator found which takes a right-hand operand of type 'initializer-list' (or there is no acceptable conversion)
Comment 9 peavo 2014-09-25 00:49:33 PDT
Created attachment 238645 [details]
Patch
Comment 10 peavo 2014-09-25 00:51:30 PDT
(In reply to comment #9)
> Created an attachment (id=238645) [details]
> Patch

It turns out MSVC can assign an initializer list to a struct, but the move constructor causes a compile error.
I removed the move constructor in the latest patch, but I'm not sure it just can be removed without other changes ...
Comment 11 peavo 2014-09-25 01:02:44 PDT
Created attachment 238647 [details]
Patch
Comment 12 peavo 2014-09-26 01:21:14 PDT
(In reply to comment #11)
> Created an attachment (id=238647) [details]
> Patch

Updated patch to fix compile errors.
Comment 13 peavo 2014-10-09 03:41:39 PDT
Apologies for nagging :) Any chance of a review?
Comment 14 peavo 2014-10-15 03:19:51 PDT
Created attachment 239862 [details]
Patch
Comment 15 peavo 2014-10-15 03:21:12 PDT
(In reply to comment #14)
> Created an attachment (id=239862) [details]
> Patch

Resolved conflicts.
Comment 16 Philippe Normand 2014-11-18 10:33:42 PST
Comment on attachment 239862 [details]
Patch

Let's cross fingers a rebase is not needed :)
Comment 17 peavo 2014-11-18 10:36:00 PST
(In reply to comment #16)
> Comment on attachment 239862 [details]
> Patch
> 
> Let's cross fingers a rebase is not needed :)

Thanks for reviewing!
Comment 18 WebKit Commit Bot 2014-11-18 11:10:15 PST
Comment on attachment 239862 [details]
Patch

Clearing flags on attachment: 239862

Committed r176269: <http://trac.webkit.org/changeset/176269>
Comment 19 WebKit Commit Bot 2014-11-18 11:10:21 PST
All reviewed patches have been landed.  Closing bug.