Convert Pass()→std::move() in //net

Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org

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

[dcheng, 2015-12-26 02:14:19]

dcheng@chromium.org changed reviewers:
+ avi@chromium.org, gavinp@chromium.org

[dcheng, 2015-12-26 02:14:21]

+avi for sanity check, +gavinp for owners review

[dcheng, 2015-12-26 02:14:35]

Description was changed from

==========
Convert Pass()→std::move() in //net

BUG=557422
==========

to

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

[dcheng, 2015-12-26 02:14:41]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-26 02:15:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1545233002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1545233002/1

[mmenke, 2015-12-26 02:33:55]

On 2015/12/26 02:15:08, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1545233002/1
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1545233002/1

It's been suggested on chromium-dev that using std::move instead of Pass in
cases where we rely on the state of the scoped_ptr after the Pass/std::move call
isn't a great idea, although it works.  That would suggest this sort of
wholesale replacement isn't a great idea.  Or did I misunderstand the
discussion?

[mmenke, 2015-12-26 02:34:29]

Description was changed from

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

to

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

[mmenke, 2015-12-26 02:34:29]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[Avi (use Gerrit), 2015-12-26 02:54:25]

lgtm

stampity stamp

[dcheng, 2015-12-26 02:59:07]

On 2015/12/26 at 02:33:55, mmenke wrote:
> On 2015/12/26 02:15:08, commit-bot: I haz the power wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1545233002/1
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1545233002/1
> 
> It's been suggested on chromium-dev that using std::move instead of Pass in
cases where we rely on the state of the scoped_ptr after the Pass/std::move call
isn't a great idea, although it works.  That would suggest this sort of
wholesale replacement isn't a great idea.  Or did I misunderstand the
discussion?

The standard guarantees that a move-from std::unique_ptr will return nullptr for
get().

In general, it is poor practice to do anything with a moved-from variable other
than destroy it or re-assign it. Converting Pass() to std::move() is orthogonal
to that.

[mmenke, 2015-12-26 03:08:05]

On 2015/12/26 02:59:07, dcheng wrote:
> On 2015/12/26 at 02:33:55, mmenke wrote:
> > On 2015/12/26 02:15:08, commit-bot: I haz the power wrote:
> > > Dry run: CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/1545233002/1
> > > View timeline at
> > >  https://chromium-cq-status.appspot.com/patch-timeline/1545233002/1
> > 
> > It's been suggested on chromium-dev that using std::move instead of Pass in
> cases where we rely on the state of the scoped_ptr after the Pass/std::move
call
> isn't a great idea, although it works.  That would suggest this sort of
> wholesale replacement isn't a great idea.  Or did I misunderstand the
> discussion?
> 
> The standard guarantees that a move-from std::unique_ptr will return nullptr
for
> get().
> 
> In general, it is poor practice to do anything with a moved-from variable
other
> than destroy it or re-assign it. Converting Pass() to std::move() is
orthogonal
> to that.

Maybe I'm not following, but those two comments seem contradictory.  Calling
.get() after std::move is indeed doing something other than destroying or
re-assigning the moved from variable (As is calling .reset on it, for that
matter).  Both of those were pretty common practices when using .Pass(), and I'm
unaware of either having been considered a bad idea.

[dcheng, 2015-12-26 03:28:13]

On 2015/12/26 at 03:08:05, mmenke wrote:
> On 2015/12/26 02:59:07, dcheng wrote:
> > On 2015/12/26 at 02:33:55, mmenke wrote:
> > > On 2015/12/26 02:15:08, commit-bot: I haz the power wrote:
> > > > Dry run: CQ is trying da patch. Follow status at
> > > >  https://chromium-cq-status.appspot.com/patch-status/1545233002/1
> > > > View timeline at
> > > >  https://chromium-cq-status.appspot.com/patch-timeline/1545233002/1
> > > 
> > > It's been suggested on chromium-dev that using std::move instead of Pass
in
> > cases where we rely on the state of the scoped_ptr after the Pass/std::move
call
> > isn't a great idea, although it works.  That would suggest this sort of
> > wholesale replacement isn't a great idea.  Or did I misunderstand the
> > discussion?
> > 
> > The standard guarantees that a move-from std::unique_ptr will return nullptr
for
> > get().
> > 
> > In general, it is poor practice to do anything with a moved-from variable
other
> > than destroy it or re-assign it. Converting Pass() to std::move() is
orthogonal
> > to that.
> 
> Maybe I'm not following, but those two comments seem contradictory.  Calling
.get() after std::move is indeed doing something other than destroying or
re-assigning the moved from variable (As is calling .reset on it, for that
matter).  Both of those were pretty common practices when using .Pass(), and I'm
unaware of either having been considered a bad idea.

/In general/, it's not a good idea to depend on the state of moved from objects
in the standard library unless otherwise specified, because the object is in a
valid but unspecified state.

However, the behavior for unique_ptr (and scoped_ptr) is specified. Thus there
is no contradiction.

Finally, std::move and Pass() are equivalent already: changing code to use it
won't change previously valid code to be invalid.

[commit-bot: I haz the power, 2015-12-26 03:46:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-26 03:46:19]

Dry run: This issue passed the CQ dry run.

[dcheng, 2015-12-26 03:47:09]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-12-26 03:47:13]

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

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

Description was changed from

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

to

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

[commit-bot: I haz the power, 2015-12-26 03:56:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-12-26 03:58:00]

Description was changed from

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org
==========

to

==========
Convert Pass()→std::move() in //net

BUG=557422
R=avi@chromium.org
TBR=gavinp@chromium.org

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

[commit-bot: I haz the power, 2015-12-26 03:58:01]

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