Cleanup: Initialize Value from tools/gn by moving std::string

Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org

Committed: https://crrev.com/0b310e43469a5ef54d76661ec36e4a7e1ca09d3f
Cr-Commit-Position: refs/heads/master@{#399012}

[tfarina, 2015-09-22 17:55:18]

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

[tfarina, 2015-09-22 17:55:19]

Could you split out this change?

testing/ -> phajdan.jr
tools/clang -> thakis
tools/gn -> brettw

Thanks,

[ki.stfu, 2015-09-23 05:00:02]

ki.stfu@gmail.com changed reviewers:
+ brettw@chromium.org, phajdan.jr@chromium.org, thakis@chromium.org
- darin@chromium.org, jam@chromium.org

[ki.stfu, 2015-09-23 05:01:55]

phajdan.jr@: please review changed files in testing/
thakis@: please review tools/clang/
brettw@: please review tools/gn

[brettw, 2015-09-23 05:29:10]

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h#newcode36
tools/gn/value.h:36: Value(const ParseNode* origin, std::string&& str_val)
noexcept;
Same comment from the other patch here. This is still technically disallowed by
the style guide.

[ki.stfu, 2015-09-23 05:44:10]

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h#newcode36
tools/gn/value.h:36: Value(const ParseNode* origin, std::string&& str_val)
noexcept;
On 2015/09/23 05:29:10, brettw wrote:
> Same comment from the other patch here. This is still technically disallowed
by
> the style guide.

Ok, I'll remove std::string&& version.

[ki.stfu, 2015-09-24 17:19:16]

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/1350043004/diff/1/tools/gn/value.h#newcode36
tools/gn/value.h:36: Value(const ParseNode* origin, std::string&& str_val)
noexcept;
On 2015/09/23 05:29:10, brettw wrote:
> Same comment from the other patch here. This is still technically disallowed
by
> the style guide.

Done.

[brettw, 2015-09-24 20:15:31]

https://codereview.chromium.org/1350043004/diff/20001/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/1350043004/diff/20001/tools/gn/value.h#newcode35
tools/gn/value.h:35: Value(const ParseNode* origin, const std::string& str_val);
I think without adding the && constructor, this will be a bad change since it
will prevent certain types of optimizations. I suspect we should just revert the
change to this file.

[ki.stfu, 2015-09-24 20:54:29]

https://codereview.chromium.org/1350043004/diff/20001/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/1350043004/diff/20001/tools/gn/value.h#newcode35
tools/gn/value.h:35: Value(const ParseNode* origin, const std::string& str_val);
On 2015/09/24 20:15:31, brettw wrote:
> I think without adding the && constructor, this will be a bad change since it
> will prevent certain types of optimizations. I suspect we should just revert
the
> change to this file.

Probably yes. BTW, why we don't want to use a movable version? You said it's
disallowed, but actually it's not. It just should be approved by OWNERS of
src/styleguide/c++/.

[brettw, 2015-09-24 21:41:23]

On 2015/09/24 20:54:29, ki.stfu wrote:
> Probably yes. BTW, why we don't want to use a movable version? You said it's
> disallowed, but actually it's not. It just should be approved by OWNERS of
> src/styleguide/c++/.

Right, I'm not actually opposed to asking for approval for this. It just seemed
less important than the other patch you were doing (where clearly it should use
move IMO).

[ki.stfu, 2015-09-27 07:35:26]

On 2015/09/24 21:41:23, brettw wrote:
> On 2015/09/24 20:54:29, ki.stfu wrote:
> > Probably yes. BTW, why we don't want to use a movable version? You said it's
> > disallowed, but actually it's not. It just should be approved by OWNERS of
> > src/styleguide/c++/.
> 
> Right, I'm not actually opposed to asking for approval for this. It just
seemed
> less important than the other patch you were doing (where clearly it should
use
> move IMO).

OK. I know we can't use movable ctors everywhere.

[ki.stfu, 2016-05-25 17:30:01]

Description was changed from

==========
Cleanup: Pass std::string as const reference from testing/ and tools/

Passing std::string by reference can prevent extra copying of object.

BUG=367418
TEST=
R=phajdan.jr@chromium.org,thakis@chromium.org,brettw@chromium.org
==========

to

==========
Cleanup: Pass std::string as const reference from tools/gn/

Passing std::string by reference can prevent extra copying of object.

BUG=367418
TEST=
R=brettw@chromium.org
==========

[ki.stfu, 2016-05-25 17:30:02]

ki.stfu@gmail.com changed reviewers:
- phajdan.jr@chromium.org, tfarina@chromium.org, thakis@chromium.org

[ki.stfu, 2016-05-25 17:36:54]

The patch was split:
Changes in testing/ were moved to https://codereview.chromium.org/2001093008/
Changes in tools/clang/ were moved to
https://codereview.chromium.org/2009293002/

[ki.stfu, 2016-05-31 12:43:43]

Description was changed from

==========
Cleanup: Pass std::string as const reference from tools/gn/

Passing std::string by reference can prevent extra copying of object.

BUG=367418
TEST=
R=brettw@chromium.org
==========

to

==========
Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org
==========

[ki.stfu, 2016-06-03 19:10:03]

Friendly ping

[ki.stfu, 2016-06-05 11:57:10]

ki.stfu@gmail.com changed reviewers:
+ scottmg@chromium.org, thakis@chromium.org

[ki.stfu, 2016-06-09 07:44:27]

Friendly ping

[scottmg, 2016-06-09 15:10:14]

I would have made it a const& arg so I think you'll have to wait for brettw on
this one.

[brettw, 2016-06-09 16:52:15]

lgtm

[ki.stfu, 2016-06-09 20:29:39]

The CQ bit was checked by ki.stfu@gmail.com

[commit-bot: I haz the power, 2016-06-09 20:30:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350043004/80001

[commit-bot: I haz the power, 2016-06-09 21:05:10]

Description was changed from

==========
Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org
==========

to

==========
Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-06-09 21:05:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-09 21:05:30]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 21:07:07]

Description was changed from

==========
Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org
==========

to

==========
Cleanup: Initialize Value from tools/gn by moving std::string

Previously it used string::swap, but std::move is more effective in this case.

BUG=367418
TEST=
R=brettw@chromium.org

Committed: https://crrev.com/0b310e43469a5ef54d76661ec36e4a7e1ca09d3f
Cr-Commit-Position: refs/heads/master@{#399012}
==========

[commit-bot: I haz the power, 2016-06-09 21:07:08]

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