[Extensions] Use rvalue references for SimpleFeature setters

[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286
Committed: https://crrev.com/7a54e03af255cf365d7260324d9872aa94dd7ec1
Cr-Commit-Position: refs/heads/master@{#407334}

[Devlin, 2016-07-22 18:48:42]

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

[commit-bot: I haz the power, 2016-07-22 18:49:26]

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

[commit-bot: I haz the power, 2016-07-22 19:45:38]

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

[commit-bot: I haz the power, 2016-07-22 19:45:39]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-07-22 20:15:39]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2016-07-22 20:58:24]

I have one q.

https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:111: void
set_command_line_switch(std::string command_line_switch);
(trying to understand stuff here, so this is probably naive q)
Why not std::string&& command_line_switch if we know the param is constructed in
place?
The advantage of that is someone outside of generated code cannot use l-value
anymore (including test):
string s;
...
set_command_line_switch(s)

Now, style guide says "only use these (rvalue) to define move constructors and
move assignment operators, and for perfect forwarding." So, that makes it no-go.
Is this the only reason to not use it?

[Devlin, 2016-07-22 21:27:35]

https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.h (right):

https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.h:111: void
set_command_line_switch(std::string command_line_switch);
On 2016/07/22 20:58:23, lazyboy wrote:
> (trying to understand stuff here, so this is probably naive q)
> Why not std::string&& command_line_switch if we know the param is constructed
in
> place?
> The advantage of that is someone outside of generated code cannot use l-value
> anymore (including test):
> string s;
> ...
> set_command_line_switch(s)
> 
> Now, style guide says "only use these (rvalue) to define move constructors and
> move assignment operators, and for perfect forwarding." So, that makes it
no-go.
> Is this the only reason to not use it?

Interesting idea!  Yes, style is the only reason not to use it.  WDYT?  Worth
breaking the style rule?

[lazyboy, 2016-07-22 23:42:16]

On 2016/07/22 21:27:35, Devlin wrote:
>
https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
> File extensions/common/features/simple_feature.h (right):
> 
>
https://codereview.chromium.org/2172353002/diff/1/extensions/common/features/...
> extensions/common/features/simple_feature.h:111: void
> set_command_line_switch(std::string command_line_switch);
> On 2016/07/22 20:58:23, lazyboy wrote:
> > (trying to understand stuff here, so this is probably naive q)
> > Why not std::string&& command_line_switch if we know the param is
constructed
> in
> > place?
> > The advantage of that is someone outside of generated code cannot use
l-value
> > anymore (including test):
> > string s;
> > ...
> > set_command_line_switch(s)
> > 
> > Now, style guide says "only use these (rvalue) to define move constructors
and
> > move assignment operators, and for perfect forwarding." So, that makes it
> no-go.
> > Is this the only reason to not use it?
> 
> Interesting idea!  Yes, style is the only reason not to use it.  WDYT?  Worth
> breaking the style rule?

Think it's fine to break the rule for one-off cases like this (generated code
only calls these functions other than tests).
My preference would be to use rvalue if we can. If not, then keep it as is.

dcheng@ or someone else might have better opinion here though.

lgtm

[Devlin, 2016-07-23 00:44:00]

On 2016/07/22 23:42:16, lazyboy wrote:
> Think it's fine to break the rule for one-off cases like this (generated code
> only calls these functions other than tests).
> My preference would be to use rvalue if we can. If not, then keep it as is.

SGTM.  Let's try rvalue and see if anyone complains.

[Devlin, 2016-07-23 00:44:05]

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

[commit-bot: I haz the power, 2016-07-23 00:44:42]

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

[Devlin, 2016-07-23 00:45:02]

Description was changed from

==========
[Extensions] Use pass-by-value for SimpleFeature setters

Use pass-by-value in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to std::move() the value into the SimpleFeature, thus avoiding
a copy.

BUG=280286
==========

to

==========
[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286
==========

[commit-bot: I haz the power, 2016-07-23 01:45:30]

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

[commit-bot: I haz the power, 2016-07-23 01:45:30]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-07-23 02:08:46]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-07-23 02:08:47]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2172353002/#ps20001
(title: "rvalue")

[commit-bot: I haz the power, 2016-07-23 02:09:05]

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

[commit-bot: I haz the power, 2016-07-23 03:32:55]

Description was changed from

==========
[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286
==========

to

==========
[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286
==========

[commit-bot: I haz the power, 2016-07-23 03:32:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-23 03:34:15]

Description was changed from

==========
[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286
==========

to

==========
[Extensions] Use rvalue references for SimpleFeature setters

Use rvalue references in SimpleFeature setters, rather than passing in a
const reference.  In the generated code these setters are used in, the
values are all constructed in-place, so no copy is performed. This then
allows us to use move assignment operaters in SimpleFeature, thus avoiding
a copy.

BUG=280286

Committed: https://crrev.com/7a54e03af255cf365d7260324d9872aa94dd7ec1
Cr-Commit-Position: refs/heads/master@{#407334}
==========

[commit-bot: I haz the power, 2016-07-23 03:34:16]

Patchset 2 (id:??) landed as
https://crrev.com/7a54e03af255cf365d7260324d9872aa94dd7ec1
Cr-Commit-Position: refs/heads/master@{#407334}