Deprecate Value::(Create)DeepCopy

Deprecate Value::(Create)DeepCopy

This change deprecates Value::CreateDeepCopy and Value::DeepCopy in favor of
invoking the copy constructor directly.

R=brettw@chromium.org
BUG=646113
Review-Url: https://codereview.chromium.org/2781983003
Cr-Commit-Position: refs/heads/master@{#461992}
Committed: https://chromium.googlesource.com/chromium/src/+/e964d9af67e0dc5326e2160c5fabf055a7ed10fa

[jdoerrie, 2017-03-29 09:49:12]

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

[commit-bot: I haz the power, 2017-03-29 09:49:39]

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, 2017-03-29 11:37:30]

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

[commit-bot: I haz the power, 2017-03-29 11:37:31]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-03-29 11:38:06]

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

[commit-bot: I haz the power, 2017-03-29 11:38:20]

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

[jdoerrie, 2017-03-29 11:40:14]

Hi Brett, please review.

[commit-bot: I haz the power, 2017-03-29 14:06:07]

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

[commit-bot: I haz the power, 2017-03-29 14:06:07]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-03-31 07:58:29]

Hi Brett, a gentle ping on this one.

[brettw, 2017-03-31 16:38:15]

LGTM, sorry I missed this.

https://codereview.chromium.org/2781983003/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2781983003/diff/20001/base/values.cc#newcode508
base/values.cc:508: (**dict_ptr_)[it.first] = MakeUnique<Value>(*it.second);
I've been trying to use insert/emplace more for this type of operation.

dict[...] = ... will be a default construction + assign of the mapped type, and
I don't think the compiler can optimize the extra assignment out.
  *dict_ptr_->insert(std::make_pair(it.first, MakeUnique<Value>(*it.second));

[jdoerrie, 2017-04-01 18:32:07]

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

[commit-bot: I haz the power, 2017-04-01 18:32:16]

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, 2017-04-01 20:16:35]

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

[commit-bot: I haz the power, 2017-04-01 20:16:36]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-03 07:10:26]

https://codereview.chromium.org/2781983003/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2781983003/diff/20001/base/values.cc#newcode508
base/values.cc:508: (**dict_ptr_)[it.first] = MakeUnique<Value>(*it.second);
On 2017/03/31 16:38:15, brettw (plz ping after 24h) wrote:
> I've been trying to use insert/emplace more for this type of operation.
> 
> dict[...] = ... will be a default construction + assign of the mapped type,
and
> I don't think the compiler can optimize the extra assignment out.
>   *dict_ptr_->insert(std::make_pair(it.first, MakeUnique<Value>(*it.second));

Yes, you are right. With the sysroot update last week it looks like we can
finally use emplace on std::maps. Emplace should be always as least as fast as
insert, so I'd prefer to do that instead. Furthermore, given that we iterate
over that's keys in order, we exactly know where to put it in this. Hence we
even can use emplace_hint here. I did some testing and it turns out there is a
net benefit given the hint is correct.

I will hold off landing just yet, so that you can comment on what I just said.
Maybe there is a reason not to use emplace_hint which I didn't think of.

[jdoerrie, 2017-04-04 13:40:58]

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

[commit-bot: I haz the power, 2017-04-04 13:41:12]

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, 2017-04-04 15:52:15]

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

[commit-bot: I haz the power, 2017-04-04 15:52:16]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-05 06:40:19]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-04-05 06:40:19]

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

[commit-bot: I haz the power, 2017-04-05 06:40:28]

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, 2017-04-05 06:44:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491374419260670,
"parent_rev": "8a1c41840a061c7de4dfe40a4f2f5713645d8800", "commit_rev":
"e964d9af67e0dc5326e2160c5fabf055a7ed10fa"}

[commit-bot: I haz the power, 2017-04-05 06:45:33]

Description was changed from

==========
Deprecate Value::(Create)DeepCopy

This change deprecates Value::CreateDeepCopy and Value::DeepCopy in favor of
invoking the copy constructor directly.

R=brettw@chromium.org
BUG=646113
==========

to

==========
Deprecate Value::(Create)DeepCopy

This change deprecates Value::CreateDeepCopy and Value::DeepCopy in favor of
invoking the copy constructor directly.

R=brettw@chromium.org
BUG=646113

Review-Url: https://codereview.chromium.org/2781983003
Cr-Commit-Position: refs/heads/master@{#461992}
Committed:
https://chromium.googlesource.com/chromium/src/+/e964d9af67e0dc5326e2160c5fab...
==========

[commit-bot: I haz the power, 2017-04-05 06:45:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e964d9af67e0dc5326e2160c5fab...