|
|
Created:
4 years, 3 months ago by Wez Modified:
4 years, 3 months ago CC:
tkent, ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, blink-reviews-wtf_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, Mikhail, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up some assertion code in WebKit.
- Redefine DCHECK_AT to always reference line and file; the compiler
should still optimize them out if !DCHECK_IS_ON().
- Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks.
These changes are prerequisites for the dump-on-DCHECK experiment.
BUG=596231
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/67a60716623af6f7e4571a206ce0df2e05e904f0
Cr-Commit-Position: refs/heads/master@{#414968}
Patch Set 1 : Try to fix patchset deps again #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 38 (25 generated)
Description was changed from ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 ========== to ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wez@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 wez@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.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
wez@chromium.org changed reviewers: + senorblanco@chromium.org
PTAL - thanks!
The CQ bit was checked by wez@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.
Description was changed from ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
+tkent FYI https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:189: #define DCHECK_AT(assertion, file, line) LAZY_STREAM(logging::LogMessage(file, line, #assertion).stream(), DCHECK_IS_ON() ? !(assertion) : false) Wouldn't this be clearer as #if DCHECK_IS_ON() #define DCHECK_AT(assertion, file, line) LAZY_STREAM(logging::LogMessage(file, line, #assertion).stream(), !(assertion)) #else #define DCHECK_AT(assertion, file, line) EAT_STREAM_PARAMETERS #endif i.e., same as before, but putting it behind #if DHECK_IS_ON() instead of #if ENABLE(ASSERT) ? Or am I missing something?
https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Assertions.h (right): https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Assertions.h:189: #define DCHECK_AT(assertion, file, line) LAZY_STREAM(logging::LogMessage(file, line, #assertion).stream(), DCHECK_IS_ON() ? !(assertion) : false) On 2016/08/27 23:20:21, Stephen White wrote: > Wouldn't this be clearer as > > #if DCHECK_IS_ON() > #define DCHECK_AT(assertion, file, line) LAZY_STREAM(logging::LogMessage(file, > line, #assertion).stream(), !(assertion)) > #else > #define DCHECK_AT(assertion, file, line) EAT_STREAM_PARAMETERS > #endif > > i.e., same as before, but putting it behind #if DHECK_IS_ON() instead of #if > ENABLE(ASSERT) ? Or am I missing something? The definition of DCHECK is specifically designed to ensure that |condition| (aka |assertion| in this version) is referenced even if DCHECK is off, so the compiler won't complain about unused variables that the condition would otherwise reference. We then rely on the compiler optimizing the condition out when it sees that DCHECK_IS_ON() is always true.
On 2016/08/28 01:11:36, Wez wrote: > https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/Assertions.h (right): > > https://codereview.chromium.org/2280003003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/Assertions.h:189: #define DCHECK_AT(assertion, > file, line) LAZY_STREAM(logging::LogMessage(file, line, #assertion).stream(), > DCHECK_IS_ON() ? !(assertion) : false) > On 2016/08/27 23:20:21, Stephen White wrote: > > Wouldn't this be clearer as > > > > #if DCHECK_IS_ON() > > #define DCHECK_AT(assertion, file, line) LAZY_STREAM(logging::LogMessage(file, > > line, #assertion).stream(), !(assertion)) > > #else > > #define DCHECK_AT(assertion, file, line) EAT_STREAM_PARAMETERS > > #endif > > > > i.e., same as before, but putting it behind #if DHECK_IS_ON() instead of #if > > ENABLE(ASSERT) ? Or am I missing something? > > The definition of DCHECK is specifically designed to ensure that |condition| > (aka |assertion| in this version) is referenced even if DCHECK is off, so the > compiler won't complain about unused variables that the condition would > otherwise reference. We then rely on the compiler optimizing the condition out > when it sees that DCHECK_IS_ON() is always true. Got it. LGTM
The CQ bit was checked by wez@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 commit-bot@chromium.org
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_presub...)
wez@chromium.org changed reviewers: + tkent@chromium.org
+tkent for Assertions.h OWNERS
The CQ bit was checked by tkent@chromium.org
lgtm
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tkent@chromium.org
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 ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Clean up some assertion code in WebKit. - Redefine DCHECK_AT to always reference line and file; the compiler should still optimize them out if !DCHECK_IS_ON(). - Fix inconsistent use of DCHECK_IS_ON() vs ENABLE(ASSERT) checks. These changes are prerequisites for the dump-on-DCHECK experiment. BUG=596231 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/67a60716623af6f7e4571a206ce0df2e05e904f0 Cr-Commit-Position: refs/heads/master@{#414968} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/67a60716623af6f7e4571a206ce0df2e05e904f0 Cr-Commit-Position: refs/heads/master@{#414968} |