Bug 122883 – [WK2] Convert SeccompFilters to using unique_ptr instead of OwnPtr/PassOwnPtr

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Correia (qrwteyrutiyoup)]

[WK2] Convert SeccompFilters to using unique_ptr instead of OwnPtr/PassOwnPtr

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 214340 [details]
Patch

[Anders Carlsson]

Comment on attachment 214340 [details]
Patch

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

Patch looks good, but please remove the unnecessary std::move calls.

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60
> +        return std::move(open);

No need to use std::move here. Returning a move-only object will automatically move the object.

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:93
> +    return std::move(open);

Ditto.

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:105
> +    return std::move(open);

Ditto.

> Source/WebKit2/Shared/linux/SeccompFilters/Syscall.cpp:76
> +    return std::move(syscall);

Ditto.

> Source/WebKit2/Shared/linux/SeccompFilters/Syscall.cpp:98
> +    return std::move(result);

Ditto.

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 214340 [details]
Patch

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

>> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60
>> +        return std::move(open);
> 
> No need to use std::move here. Returning a move-only object will automatically move the object.

It seems to be needed for this class: 

[ 82%] Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/Shared/linux/SeccompFilters/OpenSyscall.cpp.o
/home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp: In static member function 'static std::unique_ptr<WebKit::Syscall> WebKit::OpenSyscall::createFromOpenatContext(mcontext_t*)':
/home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: invalid conversion from 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&' [-fpermissive]
In file included from /usr/include/c++/4.8.1/memory:81:0,
                 from /home/sergio/projects/webkit/Source/WTF/wtf/StdLibExtras.h:30,
                 from /home/sergio/projects/webkit/Source/WTF/wtf/FastMalloc.h:28,
                 from /home/sergio/projects/webkit/Source/WebKit2/config.h:87,
                 from /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:26:
/usr/include/c++/4.8.1/bits/unique_ptr.h:169:2: error:   initializing argument 1 of 'std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = WebKit::OpenSyscall; _Ep = std::default_delete<WebKit::OpenSyscall>; <template-parameter-2-3> = void; _Tp = WebKit::Syscall; _Dp = std::default_delete<WebKit::Syscall>]' [-fpermissive]
/home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: cannot convert 'open' from type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&'

[Anders Carlsson]

Comment on attachment 214340 [details]
Patch

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

>>> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60
>>> +        return std::move(open);
>> 
>> No need to use std::move here. Returning a move-only object will automatically move the object.
> 
> It seems to be needed for this class: 
> 
> [ 82%] Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/Shared/linux/SeccompFilters/OpenSyscall.cpp.o
> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp: In static member function 'static std::unique_ptr<WebKit::Syscall> WebKit::OpenSyscall::createFromOpenatContext(mcontext_t*)':
> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: invalid conversion from 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&' [-fpermissive]
> In file included from /usr/include/c++/4.8.1/memory:81:0,
>                  from /home/sergio/projects/webkit/Source/WTF/wtf/StdLibExtras.h:30,
>                  from /home/sergio/projects/webkit/Source/WTF/wtf/FastMalloc.h:28,
>                  from /home/sergio/projects/webkit/Source/WebKit2/config.h:87,
>                  from /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:26:
> /usr/include/c++/4.8.1/bits/unique_ptr.h:169:2: error:   initializing argument 1 of 'std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = WebKit::OpenSyscall; _Ep = std::default_delete<WebKit::OpenSyscall>; <template-parameter-2-3> = void; _Tp = WebKit::Syscall; _Dp = std::default_delete<WebKit::Syscall>]' [-fpermissive]
> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: cannot convert 'open' from type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&'

Wait, why does OpenSyscall::createFromOpenatContext return a PassOwnPtr?

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 214340 [details]
Patch

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

>>>> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60
>>>> +        return std::move(open);
>>> 
>>> No need to use std::move here. Returning a move-only object will automatically move the object.
>> 
>> It seems to be needed for this class: 
>> 
>> [ 82%] Building CXX object Source/WebKit2/CMakeFiles/WebKit2.dir/Shared/linux/SeccompFilters/OpenSyscall.cpp.o
>> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp: In static member function 'static std::unique_ptr<WebKit::Syscall> WebKit::OpenSyscall::createFromOpenatContext(mcontext_t*)':
>> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: invalid conversion from 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&' [-fpermissive]
>> In file included from /usr/include/c++/4.8.1/memory:81:0,
>>                  from /home/sergio/projects/webkit/Source/WTF/wtf/StdLibExtras.h:30,
>>                  from /home/sergio/projects/webkit/Source/WTF/wtf/FastMalloc.h:28,
>>                  from /home/sergio/projects/webkit/Source/WebKit2/config.h:87,
>>                  from /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:26:
>> /usr/include/c++/4.8.1/bits/unique_ptr.h:169:2: error:   initializing argument 1 of 'std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = WebKit::OpenSyscall; _Ep = std::default_delete<WebKit::OpenSyscall>; <template-parameter-2-3> = void; _Tp = WebKit::Syscall; _Dp = std::default_delete<WebKit::Syscall>]' [-fpermissive]
>> /home/sergio/projects/webkit/Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60:16: error: cannot convert 'open' from type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >' to type 'std::unique_ptr<WebKit::OpenSyscall, std::default_delete<WebKit::OpenSyscall> >&&'
> 
> Wait, why does OpenSyscall::createFromOpenatContext return a PassOwnPtr?

I don't see any special reason, other than the fact that Syscall::createFromContext() returns a PassOwnPtr, and these create-from-context calls such as OpenSyscall::createFromOpenatContext() do so as well. It could probably be reworked not to do that. Would you like me to attempt something in that direction?

[Anders Carlsson]

(In reply to comment #5)
> > Wait, why does OpenSyscall::createFromOpenatContext return a PassOwnPtr?
> 
> I don't see any special reason, other than the fact that Syscall::createFromContext() returns a PassOwnPtr, and these create-from-context calls such as OpenSyscall::createFromOpenatContext() do so as well. It could probably be reworked not to do that. Would you like me to attempt something in that direction?

Yes, please make all of these return std::unique_ptr.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #6)
> (In reply to comment #5)
> > > Wait, why does OpenSyscall::createFromOpenatContext return a PassOwnPtr?
> > 
> > I don't see any special reason, other than the fact that Syscall::createFromContext() returns a PassOwnPtr, and these create-from-context calls such as OpenSyscall::createFromOpenatContext() do so as well. It could probably be reworked not to do that. Would you like me to attempt something in that direction?
> 
> Yes, please make all of these return std::unique_ptr.

Wait, there's a minor misunderstanding here. The original patch did that (that was the goal, to convert to using unique_ptr), but you pointed out that I wouldn't need the std::move() because the objects would be moved automatically.
Then I said that I would still need the std::move() calls in OpenSyscall.cpp. The PassOwnPtr you were looking at were from the current code, before conversion. Let me post an updated patch removing the unecessary std::move() calls from Syscall.cpp.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 214596 [details]
=v2

Removed unecessary std::move() calls in Syscall.cpp.

[Darin Adler]

Comment on attachment 214596 [details]
=v2

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

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:60
> +        return std::move(open);

No need for std::move here.

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:93
> +    return std::move(open);

No need for std::move here.

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:105
> +    return std::move(open);

No need for std::move here.

[Darin Adler]

(In reply to comment #7)
> Then I said that I would still need the std::move() calls in OpenSyscall.cpp.

Why? I don’t get it?

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #10)
> (In reply to comment #7)
> > Then I said that I would still need the std::move() calls in OpenSyscall.cpp.
> 
> Why? I don’t get it?

This question has insights on the issue at hand:
http://stackoverflow.com/questions/17481018/when-is-explicit-move-needed-for-a-return-statement

Basically we need the explicit move in there because the return type is std::unique_ptr<Syscall> and we are returning a std::unique_ptr<OpenSyscall>. The compiler message is this: 

error: invalid conversion from 'std::unique_ptr<WebKit::OpenSyscall>' to 'std::unique_ptr<WebKit::OpenSyscall>&&'

[Sergio Correia (qrwteyrutiyoup)]

Hi Darin/Anders,

could you please take another look at this?

[WebKit Commit Bot]

Comment on attachment 214596 [details]
=v2

Clearing flags on attachment: 214596

Committed r158388: <http://trac.webkit.org/changeset/158388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.