Use std::move() instead of scoped_ptr<>::Pass().

Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.

Committed: https://crrev.com/aa6fa234d0649238dfddf6eb7aa80d0ddc71e66f
Cr-Commit-Position: refs/heads/master@{#366687}

[Sergey Ulanov, 2015-12-19 00:19:52]

Description was changed from

==========
Use std::move() instead of scoped_ptr<>::Pass().

There is presubmit check that doesn't allow Pass() anymore.
==========

to

==========
Use std::move() instead of scoped_ptr<>::Pass().

Now there is a presubmit check that doesn't allow Pass() anymore.
==========

[Sergey Ulanov, 2015-12-19 00:19:59]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-12-19 00:20:33]

sergeyu@chromium.org changed reviewers:
+ joedow@chromium.org

[Sergey Ulanov, 2015-12-19 00:20:34]

Will update other parts of src/remoting in separate CLs

[Sergey Ulanov, 2015-12-21 18:22:13]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org
- joedow@chromium.org

[Sergey Ulanov, 2015-12-21 18:22:14]

Joe is on vacation
+jamiewalch -joedow

[Jamie, 2015-12-22 12:23:36]

Jamie is also on vacation :)

I think a reference to https://www.chromium.org/rvalue-references in the CL
description would be useful for context.

Also, there are discrepancies in return statements. If they should all be
without std::move, then LGTM once the ones that call it are fixed (I called out
the first few, but there are others). If there's some reason why it's needed in
some cases but not others, then I have concerns that doing the wrong thing could
lead to subtle bugs.

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_authenticator.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_authenticator.cc:221: return std::move(authenticator);
I don't think you need move here
(https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D...)

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
File remoting/protocol/ice_connection_to_client.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
remoting/protocol/ice_connection_to_client.cc:98: return std::move(pump);
Unnecessary?

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:84: return
std::move(result);
Unnecessary?

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:104: return
std::move(result);
Unnecessary?

[Sergey Ulanov, 2015-12-22 16:52:24]

Description was changed from

==========
Use std::move() instead of scoped_ptr<>::Pass().

Now there is a presubmit check that doesn't allow Pass() anymore.
==========

to

==========
Use std::move() instead of scoped_ptr<>::Pass().

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

[Sergey Ulanov, 2015-12-22 17:02:44]

On 2015/12/22 12:23:36, Jamie wrote:
> Jamie is also on vacation :)

Ah, it wasn't marked in your calendar, so I wasn't sure.


> I think a reference to https://www.chromium.org/rvalue-references in the CL
> description would be useful for context.

Added a link.

> 
> Also, there are discrepancies in return statements. If they should all be
> without std::move, then LGTM once the ones that call it are fixed (I called
out
> the first few, but there are others). If there's some reason why it's needed
in
> some cases but not others, then I have concerns that doing the wrong thing
could
> lead to subtle bugs.

std::move() is required in return statement when upcasting scoped_ptr<>
(scoped_ptr<> should be covariant, but the compiler doesn't understand that, so
creation of a new scoped_ptr<> instance is required when upcasting). This case
is called out in https://www.chromium.org/rvalue-references . I don't think this
can create any bugs. Compiler warns when move() is used in return statements
where it's not necessary (if I understand correctly the generated code may be a
bit less efficient in that case).

[Sergey Ulanov, 2015-12-22 18:07:35]

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_authenticator.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_authenticator.cc:221: return std::move(authenticator);
On 2015/12/22 12:23:36, Jamie wrote:
> I don't think you need move here
>
(https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D...)

This return statement requires upcast from FakeAuthenticator to Authenticator,
so it doesn't compile without move(). It's mentioned on the same page:

   However, if the types of the variable and the return type do 
   not match exactly, then you will have to use std::move() in 
   order to convert without creating an extra temporary.

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
File remoting/protocol/ice_connection_to_client.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
remoting/protocol/ice_connection_to_client.cc:98: return std::move(pump);
On 2015/12/22 12:23:36, Jamie wrote:
> Unnecessary?

Upcasting to VideoStream.

BTW the compiler (at least clang) throws a warning when using unnecessary
std::move() in a return statement.

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:84: return
std::move(result);
On 2015/12/22 12:23:36, Jamie wrote:
> Unnecessary?

Necessary because upcasting.

https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
remoting/protocol/me2me_host_authenticator_factory.cc:104: return
std::move(result);
On 2015/12/22 12:23:36, Jamie wrote:
> Unnecessary?

Necessary because upcasting.

[Sergey Ulanov, 2015-12-22 18:24:51]

Description was changed from

==========
Use std::move() instead of scoped_ptr<>::Pass().

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

to

==========
Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

[Jamie, 2015-12-22 22:13:24]

On 2015/12/22 18:07:35, Sergey Ulanov wrote:
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
> File remoting/protocol/fake_authenticator.cc (right):
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/fake_...
> remoting/protocol/fake_authenticator.cc:221: return std::move(authenticator);
> On 2015/12/22 12:23:36, Jamie wrote:
> > I don't think you need move here
> >
>
(https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D...)
> 
> This return statement requires upcast from FakeAuthenticator to Authenticator,
> so it doesn't compile without move(). It's mentioned on the same page:
> 
>    However, if the types of the variable and the return type do 
>    not match exactly, then you will have to use std::move() in 
>    order to convert without creating an extra temporary.
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
> File remoting/protocol/ice_connection_to_client.cc (right):
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/ice_c...
> remoting/protocol/ice_connection_to_client.cc:98: return std::move(pump);
> On 2015/12/22 12:23:36, Jamie wrote:
> > Unnecessary?
> 
> Upcasting to VideoStream.
> 
> BTW the compiler (at least clang) throws a warning when using unnecessary
> std::move() in a return statement.
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
> File remoting/protocol/me2me_host_authenticator_factory.cc (right):
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
> remoting/protocol/me2me_host_authenticator_factory.cc:84: return
> std::move(result);
> On 2015/12/22 12:23:36, Jamie wrote:
> > Unnecessary?
> 
> Necessary because upcasting.
> 
>
https://codereview.chromium.org/1534193004/diff/20001/remoting/protocol/me2me...
> remoting/protocol/me2me_host_authenticator_factory.cc:104: return
> std::move(result);
> On 2015/12/22 12:23:36, Jamie wrote:
> > Unnecessary?
> 
> Necessary because upcasting.

As long as there's compiler support to indicate where it is and isn't needed, I
withdraw my objection. LGTM.

[Sergey Ulanov, 2015-12-22 22:53:22]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-12-22 22:53:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534193004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534193004/20001

[commit-bot: I haz the power, 2015-12-22 23:26:55]

Description was changed from

==========
Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

to

==========
Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

[commit-bot: I haz the power, 2015-12-22 23:26:56]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2015-12-22 23:27:45]

Description was changed from

==========
Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.
==========

to

==========
Use std::move() instead of scoped_ptr<>::Pass() in remoting/protocol

Now there is a presubmit check that doesn't allow Pass() anymore.
See https://www.chromium.org/rvalue-references for information
about std::move in chromium.

Committed: https://crrev.com/aa6fa234d0649238dfddf6eb7aa80d0ddc71e66f
Cr-Commit-Position: refs/heads/master@{#366687}
==========

[commit-bot: I haz the power, 2015-12-22 23:27:45]

Patchset 1 (id:??) landed as
https://crrev.com/aa6fa234d0649238dfddf6eb7aa80d0ddc71e66f
Cr-Commit-Position: refs/heads/master@{#366687}