Issue 2438073002: Use movable types for CallStackProfile(s) to remove copying of data.

bcwhite

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

4 years, 2 months ago (2016-10-20 17:09:52 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/1

4 years, 2 months ago (2016-10-20 17:10:26 UTC) #2

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 17:20:06 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/91003)

4 years, 2 months ago (2016-10-20 17:20:07 UTC) #4

bcwhite

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

4 years, 2 months ago (2016-10-20 19:16:16 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/20001

4 years, 2 months ago (2016-10-20 19:16:45 UTC) #6

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 19:35:16 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cronet/builds/21902)

4 years, 2 months ago (2016-10-20 19:35:17 UTC) #8

bcwhite

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

4 years, 2 months ago (2016-10-20 19:47:30 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/40001

4 years, 2 months ago (2016-10-20 19:47:50 UTC) #10

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 19:57:04 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/91123)

4 years, 2 months ago (2016-10-20 19:57:05 UTC) #12

bcwhite

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

4 years, 2 months ago (2016-10-20 19:58:45 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/60001

4 years, 2 months ago (2016-10-20 19:59:08 UTC) #14

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 20:26:31 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cronet/builds/21933)

4 years, 2 months ago (2016-10-20 20:26:32 UTC) #16

bcwhite

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

4 years, 2 months ago (2016-10-20 20:39:08 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/80001

4 years, 2 months ago (2016-10-20 20:39:50 UTC) #19

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 21:08:15 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/149989)

4 years, 2 months ago (2016-10-20 21:08:16 UTC) #22

bcwhite

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

4 years, 2 months ago (2016-10-21 12:58:33 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/100001

4 years, 2 months ago (2016-10-21 12:58:45 UTC) #24

bcwhite

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

4 years, 2 months ago (2016-10-21 13:06:28 UTC) #25

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/120001

4 years, 2 months ago (2016-10-21 13:06:37 UTC) #26

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 13:31:33 UTC) #28

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/300508)

4 years, 2 months ago (2016-10-21 13:31:34 UTC) #29

bcwhite

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

4 years, 2 months ago (2016-10-21 13:58:40 UTC) #30

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/140001

4 years, 2 months ago (2016-10-21 13:58:50 UTC) #31

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 14:29:17 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/300524)

4 years, 2 months ago (2016-10-21 14:29:18 UTC) #33

bcwhite

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

4 years, 2 months ago (2016-10-21 14:32:37 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/160001

4 years, 2 months ago (2016-10-21 14:32:55 UTC) #35

bcwhite

bcwhite@chromium.org changed reviewers: + wittman@chromium.org

4 years, 2 months ago (2016-10-21 16:10:35 UTC) #40

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 17:06:45 UTC) #42

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-21 17:06:46 UTC) #43

Mike Wittman

https://codereview.chromium.org/2438073002/diff/160001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2438073002/diff/160001/base/profiler/stack_sampling_profiler.cc#newcode113 base/profiler/stack_sampling_profiler.cc:113: const CallStackProfile& other) = default; nit: function order (this ...

4 years, 2 months ago (2016-10-21 18:05:30 UTC) #44

bcwhite

https://codereview.chromium.org/2438073002/diff/160001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2438073002/diff/160001/base/profiler/stack_sampling_profiler.cc#newcode113 base/profiler/stack_sampling_profiler.cc:113: const CallStackProfile& other) = default; On 2016/10/21 18:05:29, Mike ...

4 years, 1 month ago (2016-10-24 13:17:45 UTC) #46

bcwhite

bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org

4 years, 1 month ago (2016-10-24 15:07:35 UTC) #48

bcwhite

bcwhite@chromium.org changed reviewers: + jschuh@chromium.org

4 years, 1 month ago (2016-10-24 15:13:54 UTC) #50

bcwhite

+jschuh, for some reason this unittest needs a security-owner review: components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc

4 years, 1 month ago (2016-10-24 15:13:54 UTC) #51

Alexei Svitkine (slow)

Please create a crbug for this and associate it via BUG= since changes are not ...

4 years, 1 month ago (2016-10-24 15:26:22 UTC) #52

bcwhite

Description was changed from ========== Use movable types for CallStackProfile(s) to remove copying of data. ...

4 years, 1 month ago (2016-10-24 21:18:30 UTC) #53

bcwhite

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

4 years, 1 month ago (2016-10-24 21:29:18 UTC) #54

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/200001

4 years, 1 month ago (2016-10-24 21:30:08 UTC) #55

bcwhite

https://codereview.chromium.org/2438073002/diff/180001/base/profiler/stack_sampling_profiler.h File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2438073002/diff/180001/base/profiler/stack_sampling_profiler.h#newcode136 base/profiler/stack_sampling_profiler.h:136: DISALLOW_ASSIGN(CallStackProfile); On 2016/10/24 15:26:21, Alexei Svitkine (slow) wrote: > ...

4 years, 1 month ago (2016-10-24 21:30:30 UTC) #56

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 23:38:04 UTC) #57

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/151561)

4 years, 1 month ago (2016-10-24 23:38:05 UTC) #58

Alexei Svitkine (slow)

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h#newcode38 components/metrics/call_stack_profile_metrics_provider.h:38: base::StackSamplingProfiler::CallStackProfiles profiles); On 2016/10/24 21:30:30, bcwhite wrote: > > ...

4 years, 1 month ago (2016-10-25 14:34:04 UTC) #59

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.h:38:
base::StackSamplingProfiler::CallStackProfiles profiles);
On 2016/10/24 21:30:30, bcwhite wrote:
> > Passing by non-const ref that results in a copy is usually not done in
> Chromium
> > - so this code and similarly for  Collect() in the other file ends up
looking
> > like a mistake.
> 
> CallStackProfiles doesn't allow copy so only std::move is allowed when passing
> the parameter.  The CompletedCallback function type, which is what this is,
> (stack_sampling_profiler.h:166) documents that these are move-only parameters.
> 
> The alternative is to use type&& for the parameter but that's against the
style
> guide except for (1) move ctor/assign or (2) perfect forwarding.
> 
> You could argue that this is a case of #2 but Mojo does its "move-only"
> parameters in this way (i.e. no &&) so I've elected to follow that style in
> order to have consistency.

Sorry, I don't think I'm following. But maybe I don't understand something in
C++.

How can CallStackProfiles which is a std::vector<CallStackProfile> be move only?
Doesn't vector support copy - what's preventing it to be moved?

Even then, if it was move only, I don't understand how that would work when
you're passing the type by value. As far as I understand, moving requires
modifying the original value - but how can a function call modify the original
value (when it's not passed by either pointer or non-const ref)?

For example, in this code:

base::StackSamplingProfiler::CallStackProfiles profiles;
profiles.push_back(...);
ReceiveCompletedProfiles(params, start_timestamp, profiles);

I don't understand how the profiles param can be move only. A move, if I
understand correctly, would transfer the contents of profiles into the param -
but the above code as written shouldn't modify |profiles| local var in any way -
or am I wrong?

Or did you mean the caller has to only call the function with
std::move(profiles) for the param? If so, that's not what I think of - when
reading "move only" - since if the caller doesn't do that, it still would end up
copying - and so if that's the intention it should be clearly documented.

But perhaps I'm missing something?

bcwhite

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h#newcode38 components/metrics/call_stack_profile_metrics_provider.h:38: base::StackSamplingProfiler::CallStackProfiles profiles); On 2016/10/25 14:34:04, Alexei Svitkine (slow) wrote: ...

4 years, 1 month ago (2016-10-25 15:01:04 UTC) #60

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.h:38:
base::StackSamplingProfiler::CallStackProfiles profiles);
On 2016/10/25 14:34:04, Alexei Svitkine (slow) wrote:
> On 2016/10/24 21:30:30, bcwhite wrote:
> > > Passing by non-const ref that results in a copy is usually not done in
> > Chromium
> > > - so this code and similarly for  Collect() in the other file ends up
> looking
> > > like a mistake.
> > 
> > CallStackProfiles doesn't allow copy so only std::move is allowed when
passing
> > the parameter.  The CompletedCallback function type, which is what this is,
> > (stack_sampling_profiler.h:166) documents that these are move-only
parameters.
> > 
> > The alternative is to use type&& for the parameter but that's against the
> style
> > guide except for (1) move ctor/assign or (2) perfect forwarding.
> > 
> > You could argue that this is a case of #2 but Mojo does its "move-only"
> > parameters in this way (i.e. no &&) so I've elected to follow that style in
> > order to have consistency.
> 
> Sorry, I don't think I'm following. But maybe I don't understand something in
> C++.
> 
> How can CallStackProfiles which is a std::vector<CallStackProfile> be move
only?
> Doesn't vector support copy - what's preventing it to be moved?

std::vector supports both copy and move construction.  If the latter, it just
moves some pointers around and steals the data from its parameter.

> Even then, if it was move only, I don't understand how that would work when
> you're passing the type by value. As far as I understand, moving requires
> modifying the original value - but how can a function call modify the original
> value (when it's not passed by either pointer or non-const ref)?

All true.  If it was const, it couldn't be modified at all.  Passed by value,
the original can't be modified.

That means that when calling this, the caller has to explicitly specify
std::move(var) in the parameter list.  That moves the contents into the
parameter which is then moved into its final destination.

This, incidentally, is why doing an rvalue-reference (i.e. type&&) would be
preferred since it just as lightweight as a regular reference... but is against
the style guide and would conflict with the code generated by mojo.

> For example, in this code:
> 
> base::StackSamplingProfiler::CallStackProfiles profiles;
> profiles.push_back(...);
> ReceiveCompletedProfiles(params, start_timestamp, profiles);
> 
> I don't understand how the profiles param can be move only. A move, if I
> understand correctly, would transfer the contents of profiles into the param -
> but the above code as written shouldn't modify |profiles| local var in any way
-
> or am I wrong?

You're correct.  It has to be

ReceiveCompletedProfiles(params, start_timestamp, std::move(profiles));

> Or did you mean the caller has to only call the function with
> std::move(profiles) for the param? If so, that's not what I think of - when
> reading "move only" - since if the caller doesn't do that, it still would end
up
> copying - and so if that's the intention it should be clearly documented.

It's always the way it has to be done.  There's no implicit moves allowed where
there is any possibility that the variable could be referenced again further
down -- that basically means that if it has a name (rather than the result of an
expression), then std::move() is required to pass to a move-only (i.e. has move
constructor but no copy constructor) parameter.

std::unique_ptr is the best example of this.  You can never pass it as a
parameter without an explicit std::move.

Alexei Svitkine (slow)

LGTM % comment https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h#newcode38 components/metrics/call_stack_profile_metrics_provider.h:38: base::StackSamplingProfiler::CallStackProfiles profiles); On 2016/10/25 15:01:04, bcwhite ...

4 years, 1 month ago (2016-10-25 15:21:18 UTC) #61

LGTM % comment

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.h:38:
base::StackSamplingProfiler::CallStackProfiles profiles);
On 2016/10/25 15:01:04, bcwhite wrote:
> On 2016/10/25 14:34:04, Alexei Svitkine (slow) wrote:
> > On 2016/10/24 21:30:30, bcwhite wrote:
> > > > Passing by non-const ref that results in a copy is usually not done in
> > > Chromium
> > > > - so this code and similarly for  Collect() in the other file ends up
> > looking
> > > > like a mistake.
> > > 
> > > CallStackProfiles doesn't allow copy so only std::move is allowed when
> passing
> > > the parameter.  The CompletedCallback function type, which is what this
is,
> > > (stack_sampling_profiler.h:166) documents that these are move-only
> parameters.
> > > 
> > > The alternative is to use type&& for the parameter but that's against the
> > style
> > > guide except for (1) move ctor/assign or (2) perfect forwarding.
> > > 
> > > You could argue that this is a case of #2 but Mojo does its "move-only"
> > > parameters in this way (i.e. no &&) so I've elected to follow that style
in
> > > order to have consistency.
> > 
> > Sorry, I don't think I'm following. But maybe I don't understand something
in
> > C++.
> > 
> > How can CallStackProfiles which is a std::vector<CallStackProfile> be move
> only?
> > Doesn't vector support copy - what's preventing it to be moved?
> 
> std::vector supports both copy and move construction.  If the latter, it just
> moves some pointers around and steals the data from its parameter.
> 
> 
> > Even then, if it was move only, I don't understand how that would work when
> > you're passing the type by value. As far as I understand, moving requires
> > modifying the original value - but how can a function call modify the
original
> > value (when it's not passed by either pointer or non-const ref)?
> 
> All true.  If it was const, it couldn't be modified at all.  Passed by value,
> the original can't be modified.
> 
> That means that when calling this, the caller has to explicitly specify
> std::move(var) in the parameter list.  That moves the contents into the
> parameter which is then moved into its final destination.
> 
> This, incidentally, is why doing an rvalue-reference (i.e. type&&) would be
> preferred since it just as lightweight as a regular reference... but is
against
> the style guide and would conflict with the code generated by mojo.
> 
> 
> > For example, in this code:
> > 
> > base::StackSamplingProfiler::CallStackProfiles profiles;
> > profiles.push_back(...);
> > ReceiveCompletedProfiles(params, start_timestamp, profiles);
> > 
> > I don't understand how the profiles param can be move only. A move, if I
> > understand correctly, would transfer the contents of profiles into the param
-
> > but the above code as written shouldn't modify |profiles| local var in any
way
> -
> > or am I wrong?
> 
> You're correct.  It has to be
> 
> ReceiveCompletedProfiles(params, start_timestamp, std::move(profiles));
> 
> 
> > Or did you mean the caller has to only call the function with
> > std::move(profiles) for the param? If so, that's not what I think of - when
> > reading "move only" - since if the caller doesn't do that, it still would
end
> up
> > copying - and so if that's the intention it should be clearly documented.
> 
> It's always the way it has to be done.  There's no implicit moves allowed
where
> there is any possibility that the variable could be referenced again further
> down -- that basically means that if it has a name (rather than the result of
an
> expression), then std::move() is required to pass to a move-only (i.e. has
move
> constructor but no copy constructor) parameter.
> 
> std::unique_ptr is the best example of this.  You can never pass it as a
> parameter without an explicit std::move.

OK, so given the above, please add a comment to each function where you have
such a parameter that mentions that it's passed by value so that the caller can
use std::move() to avoid copies.

This way, having such a comment makes it explicit that it's intentional that
it's being passed by value (and not a mistake of someone forgetting to put
const&).

bcwhite

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

4 years, 1 month ago (2016-10-25 16:58:43 UTC) #62

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/220001

4 years, 1 month ago (2016-10-25 16:59:12 UTC) #63

bcwhite

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

4 years, 1 month ago (2016-10-25 17:15:56 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/240001

4 years, 1 month ago (2016-10-25 17:16:17 UTC) #66

bcwhite

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2438073002/diff/180001/components/metrics/call_stack_profile_metrics_provider.h#newcode38 components/metrics/call_stack_profile_metrics_provider.h:38: base::StackSamplingProfiler::CallStackProfiles profiles); On 2016/10/25 15:21:18, Alexei Svitkine (slow) wrote: ...

4 years, 1 month ago (2016-10-25 17:16:21 UTC) #67

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/2438073002/diff/180001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.h:38:
base::StackSamplingProfiler::CallStackProfiles profiles);
On 2016/10/25 15:21:18, Alexei Svitkine (slow) wrote:
> On 2016/10/25 15:01:04, bcwhite wrote:
> > On 2016/10/25 14:34:04, Alexei Svitkine (slow) wrote:
> > > On 2016/10/24 21:30:30, bcwhite wrote:
> > > > > Passing by non-const ref that results in a copy is usually not done in
> > > > Chromium
> > > > > - so this code and similarly for  Collect() in the other file ends up
> > > looking
> > > > > like a mistake.
> > > > 
> > > > CallStackProfiles doesn't allow copy so only std::move is allowed when
> > passing
> > > > the parameter.  The CompletedCallback function type, which is what this
> is,
> > > > (stack_sampling_profiler.h:166) documents that these are move-only
> > parameters.
> > > > 
> > > > The alternative is to use type&& for the parameter but that's against
the
> > > style
> > > > guide except for (1) move ctor/assign or (2) perfect forwarding.
> > > > 
> > > > You could argue that this is a case of #2 but Mojo does its "move-only"
> > > > parameters in this way (i.e. no &&) so I've elected to follow that style
> in
> > > > order to have consistency.
> > > 
> > > Sorry, I don't think I'm following. But maybe I don't understand something
> in
> > > C++.
> > > 
> > > How can CallStackProfiles which is a std::vector<CallStackProfile> be move
> > only?
> > > Doesn't vector support copy - what's preventing it to be moved?
> > 
> > std::vector supports both copy and move construction.  If the latter, it
just
> > moves some pointers around and steals the data from its parameter.
> > 
> > 
> > > Even then, if it was move only, I don't understand how that would work
when
> > > you're passing the type by value. As far as I understand, moving requires
> > > modifying the original value - but how can a function call modify the
> original
> > > value (when it's not passed by either pointer or non-const ref)?
> > 
> > All true.  If it was const, it couldn't be modified at all.  Passed by
value,
> > the original can't be modified.
> > 
> > That means that when calling this, the caller has to explicitly specify
> > std::move(var) in the parameter list.  That moves the contents into the
> > parameter which is then moved into its final destination.
> > 
> > This, incidentally, is why doing an rvalue-reference (i.e. type&&) would be
> > preferred since it just as lightweight as a regular reference... but is
> against
> > the style guide and would conflict with the code generated by mojo.
> > 
> > 
> > > For example, in this code:
> > > 
> > > base::StackSamplingProfiler::CallStackProfiles profiles;
> > > profiles.push_back(...);
> > > ReceiveCompletedProfiles(params, start_timestamp, profiles);
> > > 
> > > I don't understand how the profiles param can be move only. A move, if I
> > > understand correctly, would transfer the contents of profiles into the
param
> -
> > > but the above code as written shouldn't modify |profiles| local var in any
> way
> > -
> > > or am I wrong?
> > 
> > You're correct.  It has to be
> > 
> > ReceiveCompletedProfiles(params, start_timestamp, std::move(profiles));
> > 
> > 
> > > Or did you mean the caller has to only call the function with
> > > std::move(profiles) for the param? If so, that's not what I think of -
when
> > > reading "move only" - since if the caller doesn't do that, it still would
> end
> > up
> > > copying - and so if that's the intention it should be clearly documented.
> > 
> > It's always the way it has to be done.  There's no implicit moves allowed
> where
> > there is any possibility that the variable could be referenced again further
> > down -- that basically means that if it has a name (rather than the result
of
> an
> > expression), then std::move() is required to pass to a move-only (i.e. has
> move
> > constructor but no copy constructor) parameter.
> > 
> > std::unique_ptr is the best example of this.  You can never pass it as a
> > parameter without an explicit std::move.
> 
> OK, so given the above, please add a comment to each function where you have
> such a parameter that mentions that it's passed by value so that the caller
can
> use std::move() to avoid copies.
> 
> This way, having such a comment makes it explicit that it's intentional that
> it's being passed by value (and not a mistake of someone forgetting to put
> const&).

Done.

bcwhite

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

4 years, 1 month ago (2016-10-25 17:20:13 UTC) #68

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/260001

4 years, 1 month ago (2016-10-25 17:20:43 UTC) #70

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-25 18:40:16 UTC) #71

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 1 month ago (2016-10-25 18:40:17 UTC) #72

bcwhite

Description was changed from ========== Use movable types for CallStackProfile(s) to remove copying of data. ...

4 years, 1 month ago (2016-10-26 18:19:36 UTC) #73

bcwhite

The patchset sent to the CQ was uploaded after l-g-t-m from wittman@chromium.org, asvitkine@chromium.org Link to ...

4 years, 1 month ago (2016-10-26 18:19:58 UTC) #75

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438073002/260001

4 years, 1 month ago (2016-10-26 18:20:47 UTC) #76

commit-bot: I haz the power

Description was changed from ========== Use movable types for CallStackProfile(s) to remove copying of data. ...

4 years, 1 month ago (2016-10-26 21:21:05 UTC) #77

commit-bot: I haz the power

Description was changed from ========== Use movable types for CallStackProfile(s) to remove copying of data. ...

4 years, 1 month ago (2016-10-26 21:23:15 UTC) #79

commit-bot: I haz the power

4 years, 1 month ago (2016-10-26 21:23:16 UTC) #80

Message was sent while issue was closed.

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

Issue 2438073002: Use movable types for CallStackProfile(s) to remove copying of data. (Closed)

Description

Patch Set 1 #

Patch Set 2 : fix tests and build problems #

Patch Set 3 : addressed review comments by wittman #

Patch Set 4 : address review comments by asvitkine #

Patch Set 5 : added some comments about std::move #

Messages