Convert Pass()→std::move() in //mojo/public/

Convert Pass()→std::move() in //mojo/public/

BUG=557422
TBR=ben@chromium.org

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

[dcheng, 2015-12-18 09:18:28]

Description was changed from

==========
Convert Pass()→std::move() in mojo/public/

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

to

==========
Convert Pass()→std::move() in //mojo/public/

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

[dcheng, 2015-12-18 09:18:38]

dcheng@chromium.org changed reviewers:
+ ben@chromium.org

[dcheng, 2015-12-18 09:18:39]

Sorry, all the other //mojo OWNERS appear to be OOO due to the holiday season.

[dcheng, 2015-12-18 09:18:43]

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

[commit-bot: I haz the power, 2015-12-18 09:18:59]

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

[commit-bot: I haz the power, 2015-12-18 09:25:56]

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

[commit-bot: I haz the power, 2015-12-18 09:25:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[dcheng, 2015-12-18 09:54:41]

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/system/...
File mojo/public/cpp/system/tests/core_unittest.cc (left):

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/system/...
mojo/public/cpp/system/tests/core_unittest.cc:487: buffer1 = buffer1.Pass();
Clang has a -Wself-move warning that this triggers. In particular, per
http://en.cppreference.com/w/cpp/utility/move, standard library objects called
with xvalue arguments (e.g. the result of std::move()) are allowed to assume
that a xvalue argument has the only reference to the object. This is obviously
not a standard library object, but given that convention, most move
operators/assignments don't check for self-assignment.

If it's critical to retain this test (and the one in macros_unittest.cc), it's
possible to test this with some shenanigans. Let me know what you think.

[dcheng, 2015-12-18 09:54:48]

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

[commit-bot: I haz the power, 2015-12-18 09:55:36]

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

[commit-bot: I haz the power, 2015-12-18 10:08:46]

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

[commit-bot: I haz the power, 2015-12-18 10:08:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[dcheng, 2015-12-21 18:34:11]

dcheng@chromium.org changed reviewers:
+ fsamuel@chromium.org, rockot@chromium.org

[dcheng, 2015-12-21 18:34:12]

+fsamuel, +rockot: can one of you look at this patch? All the //mojo OWNERS are
on hiatus during the holiday weeks.

[Ken Rockot(use gerrit already), 2015-12-21 18:49:22]

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/array.h (right):

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/array.h:59: static Array New(size_t size) { return
std::move(Array(size)); }
Shouldn't clang complain that moving a temporary prevents copy elision (here and
elsewhere)? Why is this necessary? I remember we were calling Pass() before
because MSVC choked otherwise, but I didn't know if that was still an issue.

[dcheng, 2015-12-21 19:27:28]

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/array.h (right):

https://codereview.chromium.org/1535943002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/array.h:59: static Array New(size_t size) { return
std::move(Array(size)); }
On 2015/12/21 at 18:49:22, Ken Rockot wrote:
> Shouldn't clang complain that moving a temporary prevents copy elision (here
and elsewhere)? Why is this necessary? I remember we were calling Pass() before
because MSVC choked otherwise, but I didn't know if that was still an issue.

It does in many instances: you'll notice that some of the diffs just deleted
Pass() instead of changing it to std::move().

However, I'm not sure why this particular instance didn't trigger the warning:
it certainly looks like it should. We had similar issues with scoped_ptr.h which
were diagnosed as a failure to have an actual move constructor, but that doesn't
appear to be the case here.

As these patches are largely automated, I'd prefer not to fix things the
compiler doesn't warn about (though this particular instance is quite odd, and
I'll be follow up on it separately anyway).

[Ken Rockot(use gerrit already), 2015-12-21 19:28:25]

OK. If it doesn't break the compile, I don't see a major issue. LGTM and thanks
for doing this!

[dcheng, 2015-12-21 19:43:29]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-12-21 19:43:37]

Description was changed from

==========
Convert Pass()→std::move() in //mojo/public/

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

to

==========
Convert Pass()→std::move() in //mojo/public/

BUG=557422
TBR=ben@chromium.org
==========

[commit-bot: I haz the power, 2015-12-21 19:44:14]

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

[commit-bot: I haz the power, 2015-12-21 19:46:36]

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

[commit-bot: I haz the power, 2015-12-21 19:46:37]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

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

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

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

Avi, mind sanity checking this before I re-CQ it?

[dcheng, 2015-12-26 01:14:10]

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

[commit-bot: I haz the power, 2015-12-26 01:14:15]

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

[commit-bot: I haz the power, 2015-12-26 02:52:29]

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

[commit-bot: I haz the power, 2015-12-26 02:52:30]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2015-12-26 04:35:47]

lgtm

stampity stamp

[dcheng, 2015-12-26 04:41:57]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-12-26 04:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1535943002/#ps60001
(title: "Regenerate correctly")

[commit-bot: I haz the power, 2015-12-26 04:42:08]

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

[commit-bot: I haz the power, 2015-12-26 04:46:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-26 04:47:39]

Description was changed from

==========
Convert Pass()→std::move() in //mojo/public/

BUG=557422
TBR=ben@chromium.org
==========

to

==========
Convert Pass()→std::move() in //mojo/public/

BUG=557422
TBR=ben@chromium.org

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

[commit-bot: I haz the power, 2015-12-26 04:47:39]

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