Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865963002:
Use MakeUnique() in base/values.cc. (Closed)
DescriptionUse MakeUnique() in base/values.cc.
Review-Url: https://codereview.chromium.org/2865963002
Cr-Commit-Position: refs/heads/master@{#470262}
Committed: https://chromium.googlesource.com/chromium/src/+/e75a641b5b0324948fc4743813bd53d1a2bb661e
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #Messages
Total messages: 18 (12 generated)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thestig@chromium.org changed reviewers: + jdoerrie@chromium.org
Thank you very much for doing this! I recently thought about doing this change myself, I'm glad you did it now :) https://codereview.chromium.org/2865963002/diff/1/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode340 base/values.cc:340: return CreateDeepCopy().release(); I'd rather like for this to stay the same. As the comment in values.h states both DeepCopy methods are deprecated in favor of using copy constructors, so I prefer the current implementation. I don't like using raw new either, but soon we should be able to delete this method completely. I am currently removing calls to the raw pointer versions of DictionaryValue::Set*, where DeepCopy has been used a lot. I will try to remove DeepCopy as a follow up to that. https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode630 base/values.cc:630: key, std::move(new_child_dictionary)); Consider using the recently added SetDictionaryWithoutPathExpansion here: child_dictionary = current_dictionary->SetDictionaryWithoutPathExpansion(key, MakeUnique<DictionaryValue>()); https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode1069 base/values.cc:1069: return CreateDeepCopy().release(); Same here. https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode1320 base/values.cc:1320: return CreateDeepCopy().release(); And here.
https://codereview.chromium.org/2865963002/diff/1/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode340 base/values.cc:340: return CreateDeepCopy().release(); On 2017/05/08 22:44:47, jdoerrie (slow this week) wrote: > I'd rather like for this to stay the same. As the comment in values.h states > both DeepCopy methods are deprecated in favor of using copy constructors, so I > prefer the current implementation. > > I don't like using raw new either, but soon we should be able to delete this > method completely. I am currently removing calls to the raw pointer versions of > DictionaryValue::Set*, where DeepCopy has been used a lot. I will try to remove > DeepCopy as a follow up to that. Done. https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode630 base/values.cc:630: key, std::move(new_child_dictionary)); On 2017/05/08 22:44:47, jdoerrie (slow this week) wrote: > Consider using the recently added SetDictionaryWithoutPathExpansion here: > > child_dictionary = current_dictionary->SetDictionaryWithoutPathExpansion(key, > MakeUnique<DictionaryValue>()); Done. Thanks. https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode1069 base/values.cc:1069: return CreateDeepCopy().release(); On 2017/05/08 22:44:47, jdoerrie (slow this week) wrote: > Same here. Done. https://codereview.chromium.org/2865963002/diff/1/base/values.cc#newcode1320 base/values.cc:1320: return CreateDeepCopy().release(); On 2017/05/08 22:44:48, jdoerrie (slow this week) wrote: > And here. Done.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494307077598300,
"parent_rev": "038181f8e93dfe554791aeedd2facbc3cd01182e", "commit_rev":
"e75a641b5b0324948fc4743813bd53d1a2bb661e"}
Message was sent while issue was closed.
Description was changed from ========== Use MakeUnique() in base/values.cc. ========== to ========== Use MakeUnique() in base/values.cc. Review-Url: https://codereview.chromium.org/2865963002 Cr-Commit-Position: refs/heads/master@{#470262} Committed: https://chromium.googlesource.com/chromium/src/+/e75a641b5b0324948fc4743813bd... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e75a641b5b0324948fc4743813bd... |
