Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(377)

Issue 2258713003: Re-write many calls to WrapUnique() with MakeUnique() (Closed)

Created:
4 years, 4 months ago by Adam Rice
Modified:
4 years, 3 months ago
Reviewers:
bcwhite, robliao, Nico
CC:
chromium-reviews, sadrul, tfarina, robliao+watch_chromium.org, gab+watch_chromium.org, asvitkine+watch_chromium.org, wfh+watch_chromium.org, tracing+reviews_chromium.org, fdoray+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Re-write many calls to WrapUnique() with MakeUnique() A mostly-automated change to convert instances of WrapUnique(new Foo(...)) to MakeUnique<Foo>(...). See the thread at https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k for background. To avoid requiring too many manual fixups, the change skips some cases that are frequently problematic. In particular, in methods named Foo::Method() it will not try to change WrapUnique(new Foo()) to MakeUnique<Foo>(). This is because Foo::Method() may be accessing an internal constructor of Foo. Cases where MakeUnique<NestedClass>(...) is called within a method of OuterClass are common but hard to detect automatically, so have been fixed-up manually. The only types of manual fix ups applied are: 1) Revert MakeUnique back to WrapUnique 2) Change NULL to nullptr in argument list (MakeUnique cannot forward NULL correctly) 3) Add base:: namespace qualifier where missing. WrapUnique(new Foo) has not been converted to MakeUnique<Foo>() as this might change behaviour if Foo does not have a user-defined constructor. For example, WrapUnique(new int) creates an unitialised integer, but MakeUnique<int>() creates an integer initialised to 0. git cl format has been been run over the CL. Spot-checking has uncovered no cases of mis-formatting. BUG=637812 Committed: https://crrev.com/ec7c3997e21fc154ab89a26779d00220c48324f4 Cr-Commit-Position: refs/heads/master@{#418167}

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 4

Patch Set 3 : Fix two nested WrapUnique()s #

Patch Set 4 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -139 lines) Patch
M base/containers/mru_cache_unittest.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M base/debug/activity_analyzer.cc View 1 chunk +3 lines, -4 lines 0 comments Download
M base/debug/activity_analyzer_unittest.cc View 2 chunks +3 lines, -4 lines 0 comments Download
M base/debug/activity_tracker.cc View 1 2 3 2 chunks +3 lines, -4 lines 0 comments Download
M base/debug/activity_tracker_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M base/files/important_file_writer_unittest.cc View 1 2 3 4 chunks +4 lines, -4 lines 0 comments Download
M base/json/json_writer_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M base/message_loop/message_pump_perftest.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M base/metrics/histogram.cc View 1 chunk +1 line, -2 lines 0 comments Download
M base/metrics/persistent_histogram_allocator.cc View 6 chunks +14 lines, -14 lines 0 comments Download
M base/metrics/persistent_histogram_allocator_unittest.cc View 1 2 3 2 chunks +4 lines, -6 lines 0 comments Download
M base/metrics/persistent_sample_map_unittest.cc View 1 2 1 chunk +5 lines, -5 lines 0 comments Download
M base/task_scheduler/priority_queue_unittest.cc View 1 chunk +8 lines, -8 lines 0 comments Download
M base/task_scheduler/scheduler_service_thread.cc View 1 2 3 1 chunk +4 lines, -6 lines 0 comments Download
M base/task_scheduler/scheduler_worker_pool_impl.cc View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M base/task_scheduler/scheduler_worker_pool_impl_unittest.cc View 5 chunks +10 lines, -10 lines 0 comments Download
M base/task_scheduler/scheduler_worker_unittest.cc View 1 2 3 1 chunk +2 lines, -4 lines 0 comments Download
M base/task_scheduler/sequence_unittest.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M base/task_scheduler/task_scheduler_impl.cc View 1 chunk +1 line, -1 line 0 comments Download
M base/task_scheduler/task_tracker_unittest.cc View 1 2 3 5 chunks +22 lines, -22 lines 0 comments Download
M base/test/test_discardable_memory_allocator.cc View 1 chunk +1 line, -1 line 0 comments Download
M base/test/test_mock_time_task_runner.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M base/test/test_reg_util_win.cc View 1 chunk +1 line, -1 line 0 comments Download
M base/trace_event/memory_dump_manager.cc View 1 1 chunk +4 lines, -4 lines 0 comments Download
M base/trace_event/process_memory_dump.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M base/trace_event/trace_config.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/trace_event/trace_event_argument_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M base/values_unittest.cc View 11 chunks +16 lines, -16 lines 0 comments Download

Messages

Total messages: 31 (18 generated)
Adam Rice
4 years, 4 months ago (2016-08-19 06:57:27 UTC) #6
bcwhite
lgtm
4 years, 4 months ago (2016-08-19 14:06:54 UTC) #7
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/2258713003/1
4 years, 4 months ago (2016-08-19 14:14:54 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/241817)
4 years, 4 months ago (2016-08-19 14:20:56 UTC) #11
robliao
base/task_scheduler/* lgtm
4 years, 4 months ago (2016-08-19 16:51:37 UTC) #13
Adam Rice
ping thakis for //base OWNERS.
4 years, 3 months ago (2016-08-31 08:27:44 UTC) #16
Nico
lgtm https://codereview.chromium.org/2258713003/diff/20001/base/metrics/persistent_sample_map_unittest.cc File base/metrics/persistent_sample_map_unittest.cc (right): https://codereview.chromium.org/2258713003/diff/20001/base/metrics/persistent_sample_map_unittest.cc#newcode20 base/metrics/persistent_sample_map_unittest.cc:20: WrapUnique(new LocalPersistentMemoryAllocator(bytes, 0, ""))); The inner WrapUnique() can't ...
4 years, 3 months ago (2016-09-12 14:10:17 UTC) #19
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/2258713003/40001
4 years, 3 months ago (2016-09-13 02:29:02 UTC) #22
commit-bot: I haz the power
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/67939) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
4 years, 3 months ago (2016-09-13 02:31:33 UTC) #24
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/2258713003/60001
4 years, 3 months ago (2016-09-13 02:48:34 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 3 months ago (2016-09-13 04:10:29 UTC) #28
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/ec7c3997e21fc154ab89a26779d00220c48324f4 Cr-Commit-Position: refs/heads/master@{#418167}
4 years, 3 months ago (2016-09-13 04:13:02 UTC) #30
Adam Rice
4 years, 3 months ago (2016-09-13 06:35:32 UTC) #31
Message was sent while issue was closed.
https://codereview.chromium.org/2258713003/diff/20001/base/metrics/persistent...
File base/metrics/persistent_sample_map_unittest.cc (right):

https://codereview.chromium.org/2258713003/diff/20001/base/metrics/persistent...
base/metrics/persistent_sample_map_unittest.cc:20: WrapUnique(new
LocalPersistentMemoryAllocator(bytes, 0, "")));
On 2016/09/12 14:10:17, Nico wrote:
> The inner WrapUnique() can't be a MakeUnique()? (I think your 'might use
private
> ctor' bit from the cl description doesn't apply here)

Just a quirk of the conversion process. I assumed that nested
WrapUnique()s would be rare. I was wrong.

Fixed.

https://codereview.chromium.org/2258713003/diff/20001/base/metrics/persistent...
base/metrics/persistent_sample_map_unittest.cc:26: WrapUnique(new
PersistentMemoryAllocator(
On 2016/09/12 14:10:17, Nico wrote:
> ditto

Done.

Powered by Google App Engine
This is Rietveld 408576698