Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869893003:
New Sequence/Thread checking API. (Closed)
DescriptionNew Sequence/Thread checking API.
Macro-based to be zero size cost in non-dcheck builds.
Based on discussion @ https://groups.google.com/a/chromium.org/d/topic/chromium-dev/99pKNd2kCcg/discussion
BUG=714835
Review-Url: https://codereview.chromium.org/2869893003
Cr-Commit-Position: refs/heads/master@{#470804}
Committed: https://chromium.googlesource.com/chromium/src/+/d52c912abb8accf430d7ba7e7d368b2638a4c8fa
Patch Set 1 #
Total comments: 9
Patch Set 2 : comments and format #Patch Set 3 : fix tests in non-dcheck builds, need fixture can't use lambdas #
Messages
Total messages: 42 (30 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTaL, as discussed on cr-dev :), will migrate all NonThreadSafe callers to this as follow-up.
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: 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...)
LGTM https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:35: // } I suggest adding the destructor checking to the example because there's been past confusion that the checker would implicitly check destructor but it does not. https://codereview.chromium.org/2869893003/diff/1/base/threading/thread_check... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:43: // void MyMethod() { same
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:20: // (use thread-local-storage or a third-party API that does.) s/.)/)./ ? https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:65: class SequenceChecker : public SequenceCheckerImpl { Suggestion: Add a presubmit warning (or error?) to prevent usage of SequenceChecker directly. Alternative suggestion: Put in base::internal:: namespace once all callers have been migrated to macros.
https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:65: class SequenceChecker : public SequenceCheckerImpl { On 2017/05/10 13:14:05, fdoray wrote: > Suggestion: Add a presubmit warning (or error?) to prevent usage of > SequenceChecker directly. > Alternative suggestion: Put in base::internal:: namespace once all callers have > been migrated to macros. Oups... I just saw that you added the PRESUBMIT check. The alternative suggestion remains valid.
Thanks, done and replied :) https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:20: // (use thread-local-storage or a third-party API that does.) On 2017/05/10 13:14:05, fdoray wrote: > s/.)/)./ ? Done. https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:35: // } On 2017/05/09 16:04:28, danakj wrote: > I suggest adding the destructor checking to the example because there's been > past confusion that the checker would implicitly check destructor but it does > not. Done. https://codereview.chromium.org/2869893003/diff/1/base/sequence_checker.h#new... base/sequence_checker.h:65: class SequenceChecker : public SequenceCheckerImpl { On 2017/05/10 13:14:05, fdoray wrote: > Alternative suggestion: Put in base::internal:: namespace once all callers have > been migrated to macros. What I'll do is remove ThreadChecker and ThreadCheckerDoNothing, but I'll leave ThreadCheckerImpl as it's sometimes useful when you want to use it for logic outside of DCHECKs. I won't put it in base::internal:: as that would indicate you should *never* use it which is not the intent. https://codereview.chromium.org/2869893003/diff/1/base/threading/thread_check... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2869893003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:43: // void MyMethod() { On 2017/05/09 16:04:28, danakj wrote: > same Done.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2869893003/#ps20001 (title: "comments and format")
gab@chromium.org changed reviewers: + dpranke@chromium.org
+dpranke for PRESUBMIT
The CQ bit was unchecked by gab@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by gab@chromium.org
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
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2869893003/#ps40001 (title: "fix tests in non-dcheck builds, need fixture can't use lambdas")
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gab@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": 40001, "attempt_start_ts": 1494461380216340,
"parent_rev": "4184757eab3cd7fd4ff3e4b565f325e096bbf97b", "commit_rev":
"d52c912abb8accf430d7ba7e7d368b2638a4c8fa"}
Message was sent while issue was closed.
Description was changed from ========== New Sequence/Thread checking API. Macro-based to be zero size cost in non-dcheck builds. Based on discussion @ https://groups.google.com/a/chromium.org/d/topic/chromium-dev/99pKNd2kCcg/dis... BUG=714835 ========== to ========== New Sequence/Thread checking API. Macro-based to be zero size cost in non-dcheck builds. Based on discussion @ https://groups.google.com/a/chromium.org/d/topic/chromium-dev/99pKNd2kCcg/dis... BUG=714835 Review-Url: https://codereview.chromium.org/2869893003 Cr-Commit-Position: refs/heads/master@{#470804} Committed: https://chromium.googlesource.com/chromium/src/+/d52c912abb8accf430d7ba7e7d36... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d52c912abb8accf430d7ba7e7d36... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
