Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053393002:
Clean Up Windows Thread Join Debug Code (Closed)
DescriptionClean Up Windows Thread Join Debug Code
BUG=127931
Committed: https://crrev.com/974eca1eda78d904c2aab3e5007036b61bd01d14
Cr-Commit-Position: refs/heads/master@{#399291}
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 16 (5 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053393002/1
robliao@chromium.org changed reviewers: + danakj@chromium.org
danakj: Please review this changelist. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:220: CHECK_EQ(WAIT_OBJECT_0, Can you make it a DCHECK also?
https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:220: CHECK_EQ(WAIT_OBJECT_0, On 2016/06/10 17:38:25, danakj wrote: > Can you make it a DCHECK also? I see you made a comment about this is bad so we should CHECK. It was a CHECK because it was tracking down a bug, which makes sense. Once a bug is resolved, I think we should make it back into a DCHECK.
https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:220: CHECK_EQ(WAIT_OBJECT_0, On 2016/06/10 17:41:06, danakj wrote: > On 2016/06/10 17:38:25, danakj wrote: > > Can you make it a DCHECK also? > > I see you made a comment about this is bad so we should CHECK. It was a CHECK > because it was tracking down a bug, which makes sense. Once a bug is resolved, I > think we should make it back into a DCHECK. We CHECK on the equivalent call in POSIX. https://cs.chromium.org/chromium/src/base/threading/platform_thread_posix.cc?... So it seemed reasonable to keep the CHECK here. A failure to join could mean a thread is now running around unexpectedly, which could cause a crash later down the line.
https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2053393002/diff/1/base/threading/platform_thr... base/threading/platform_thread_win.cc:220: CHECK_EQ(WAIT_OBJECT_0, On 2016/06/10 18:06:42, robliao wrote: > On 2016/06/10 17:41:06, danakj wrote: > > On 2016/06/10 17:38:25, danakj wrote: > > > Can you make it a DCHECK also? > > > > I see you made a comment about this is bad so we should CHECK. It was a CHECK > > because it was tracking down a bug, which makes sense. Once a bug is resolved, > I > > think we should make it back into a DCHECK. > > We CHECK on the equivalent call in POSIX. > https://cs.chromium.org/chromium/src/base/threading/platform_thread_posix.cc?... > > So it seemed reasonable to keep the CHECK here. A failure to join could mean a > thread is now running around unexpectedly, which could cause a crash later down > the line. Oh, okay thanks for the pointer. LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053393002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Clean Up Windows Thread Join Debug Code BUG=127931 ========== to ========== Clean Up Windows Thread Join Debug Code BUG=127931 Committed: https://crrev.com/974eca1eda78d904c2aab3e5007036b61bd01d14 Cr-Commit-Position: refs/heads/master@{#399291} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/974eca1eda78d904c2aab3e5007036b61bd01d14 Cr-Commit-Position: refs/heads/master@{#399291} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
