Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218443002:
Fix SchedulerLock unittests to actually generate a DCHECK, not an exception (Closed)
|
Created:
4 years, 4 months ago by gab Modified:
4 years, 4 months ago Reviewers:
robliao CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@b2b0_simplethreadJoinable Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix SchedulerLock unittests to actually generate a DCHECK, not an exception
in SafeAcquisitionTracker::AssertSafePredecessor().
Issue was found when writing https://codereview.chromium.org/2213933003/
BUG=553459
NO_DEPENDENCY_CHECKS=true
Committed: https://crrev.com/9ffb2552a784cbbdfd7cf39d1c3913bd5242c948
Cr-Commit-Position: refs/heads/master@{#409879}
Patch Set 1 #Patch Set 2 : tweak comment #
Total comments: 2
Patch Set 3 : re-tweak comment w/ robliao's suggestion #
Total comments: 2
Patch Set 4 : s/lock/SchedulerLock/ #Messages
Total messages: 23 (11 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Description was changed from ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 ========== to ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 NO_DEPENDENCY_CHECKS=true ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + robliao@chromium.org
@robliao: split from https://codereview.chromium.org/2213933003/
lgtm. Optional: I guess you could say by induction the lock chain is safe if the predecessor is safe.
On 2016/08/04 17:47:48, robliao wrote: > lgtm. Optional: I guess you could say by induction the lock chain is safe if the > predecessor is safe. Updated comment PTAnL
https://codereview.chromium.org/2218443002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/2218443002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_lock_impl.cc:76: // This asserts that |lock|'s registered predecessor is safe (and by Iterating over chat.
https://codereview.chromium.org/2218443002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/2218443002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_lock_impl.cc:76: // This asserts that |lock|'s registered predecessor is safe (and by On 2016/08/04 18:13:29, robliao wrote: > Iterating over chat. Done.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2218443002/#ps40001 (title: "re-tweak comment w/ robliao's suggestion")
lgtm + some more s/lock/SchedulerLock/ https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_lock_impl.cc:78: // specified on a SchedulerLock must already exist, the first registered lock s/registered lock/registered SchedulerLock/ https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_lock_impl.cc:80: // Any subsequent lock with a predecessor must come from the set of registered s/subsequent lock/subsequent SchedulerLock/, and so forth.
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 gab@chromium.org
On 2016/08/04 18:15:53, robliao wrote: > lgtm + some more s/lock/SchedulerLock/ > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > File base/task_scheduler/scheduler_lock_impl.cc (right): > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > base/task_scheduler/scheduler_lock_impl.cc:78: // specified on a SchedulerLock > must already exist, the first registered lock > s/registered lock/registered SchedulerLock/ > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > base/task_scheduler/scheduler_lock_impl.cc:80: // Any subsequent lock with a > predecessor must come from the set of registered > s/subsequent lock/subsequent SchedulerLock/, and so forth. Did them all, not convinced it reads better though. After the first sentence it's fairly obvious that "lock" is a SchedulerLock?
On 2016/08/04 18:20:43, gab wrote: > On 2016/08/04 18:15:53, robliao wrote: > > lgtm + some more s/lock/SchedulerLock/ > > > > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > > File base/task_scheduler/scheduler_lock_impl.cc (right): > > > > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > > base/task_scheduler/scheduler_lock_impl.cc:78: // specified on a SchedulerLock > > must already exist, the first registered lock > > s/registered lock/registered SchedulerLock/ > > > > > https://codereview.chromium.org/2218443002/diff/40001/base/task_scheduler/sch... > > base/task_scheduler/scheduler_lock_impl.cc:80: // Any subsequent lock with a > > predecessor must come from the set of registered > > s/subsequent lock/subsequent SchedulerLock/, and so forth. > > Did them all, not convinced it reads better though. After the first sentence > it's fairly obvious that "lock" is a SchedulerLock? Agreed that it's marginal, but at least it's consistent.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2218443002/#ps60001 (title: "s/lock/SchedulerLock/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 NO_DEPENDENCY_CHECKS=true ========== to ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 NO_DEPENDENCY_CHECKS=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 NO_DEPENDENCY_CHECKS=true ========== to ========== Fix SchedulerLock unittests to actually generate a DCHECK, not an exception in SafeAcquisitionTracker::AssertSafePredecessor(). Issue was found when writing https://codereview.chromium.org/2213933003/ BUG=553459 NO_DEPENDENCY_CHECKS=true Committed: https://crrev.com/9ffb2552a784cbbdfd7cf39d1c3913bd5242c948 Cr-Commit-Position: refs/heads/master@{#409879} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9ffb2552a784cbbdfd7cf39d1c3913bd5242c948 Cr-Commit-Position: refs/heads/master@{#409879} |
