Change DCHECK_IS_ON() to DCHECK_IS_ON

Change DCHECK_IS_ON() to DCHECK_IS_ON

DCHECK_IS_ON has been a constant since r255987, and can be used in both
if() and #if. It no more needs to be in function form.

Converted 'if (DCHECK_IS_ON)' to '#if DCHECK_IS_ON' if proper.

BUG=350462
TEST=build
TBR=darin

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257156

[Xianzhu, 2014-03-11 23:57:17]

https://codereview.chromium.org/195973002/diff/40001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_shared_resources_unittest.cc (left):

https://codereview.chromium.org/195973002/diff/40001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_shared_resources_unittest.cc:20: #if ENABLE_DCHECK
Because ENABLE_DCHECK was an internal macro defined and undefined in
base/logging.h, it was never seen by outside code, so this block was never
compiled.

[Nico, 2014-03-11 23:59:59]

should this be #if defined(DCHECK_IS_ON) or #if DCHECK_IS_ON now, to make clear
that this is a compile-time thing?

[Xianzhu, 2014-03-12 00:13:35]

On 2014/03/11 23:59:59, Nico wrote:
> should this be #if defined(DCHECK_IS_ON) or #if DCHECK_IS_ON now, to make
clear
> that this is a compile-time thing?

I wanted to let the caller choose between #if and if().
It seems better to use #if if possible. Will change them. 

(However, I'm still wondering why logging.h forces to use if() for DEBUG_MODE by
defining it as an enum value.)

[Nico, 2014-03-12 00:15:55]

Maybe that used to be controlled by a flag too?


On Tue, Mar 11, 2014 at 5:13 PM, <wangxianzhu@chromium.org> wrote:

> On 2014/03/11 23:59:59, Nico wrote:
>
>> should this be #if defined(DCHECK_IS_ON) or #if DCHECK_IS_ON now, to make
>>
> clear
>
>> that this is a compile-time thing?
>>
>
> I wanted to let the caller choose between #if and if().
> It seems better to use #if if possible. Will change them.
>
> (However, I'm still wondering why logging.h forces to use if() for
> DEBUG_MODE by
> defining it as an enum value.)
>
> https://codereview.chromium.org/195973002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Xianzhu, 2014-03-12 01:03:42]

Done converting 'if (DCHECK_IS_ON)' to '#if DCHECK_IS_ON'.

[Nico, 2014-03-12 21:16:41]

lgtm

I hope this doesn't make any net code auto-merging harder. +rch to  comment on
that.

https://codereview.chromium.org/195973002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/195973002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:58: // A helper class
that works like AutoLock but only acquires the lock when
keep this comment

https://codereview.chromium.org/195973002/diff/60001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_shared_resources_unittest.cc (left):

https://codereview.chromium.org/195973002/diff/60001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_shared_resources_unittest.cc:24:
options.bind_generates_resource = false;
(This removal was explained here:
https://codereview.chromium.org/195973002/diff/40001/gpu/command_buffer/tests...
)

[Xianzhu, 2014-03-12 21:39:12]

https://codereview.chromium.org/195973002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/195973002/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:58: // A helper class
that works like AutoLock but only acquires the lock when
On 2014/03/12 21:16:42, Nico wrote:
> keep this comment

Done.

[Xianzhu, 2014-03-13 16:14:17]

darin@, would you comment on the impact of this CL on net code auto-merging?

[Ryan Hamilton, 2014-03-13 16:32:09]

net/ LGTM.  We never use this macro in shared code, so it's no problem.

[Xianzhu, 2014-03-13 16:38:25]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-03-13 16:39:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/195973002/80001

[commit-bot: I haz the power, 2014-03-13 17:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 17:26:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Xianzhu, 2014-03-13 17:58:26]

On 2014/03/13 16:14:17, Xianzhu wrote:
> darin@, would you comment on the impact of this CL on net code auto-merging?
(Above should be rch@)

Darin, could you review the following files?
    cc/layers/delegated_frame_provider.cc
    cc/layers/picture_layer_impl.cc
    cc/trees/tree_synchronizer.cc
    content/common/gpu/media/gpu_video_decode_accelerator.cc
    content/renderer/media/webmediaplayer_impl.cc
    gpu/command_buffer/service/test_helper.cc
    gpu/command_buffer/tests/gl_shared_resources_unittest.cc
    mojo/system/dispatcher.cc
    sync/internal_api/http_bridge.cc

Thanks.

[danakj, 2014-03-13 18:04:38]

cc LGTM

[Nico, 2014-03-13 20:18:42]

I think you can go ahead and land this TBR=darin. It's a very mechanical change.

[Peter Kasting, 2014-03-13 20:21:22]

FWIW, if you guys think "#if X" is preferable to "#if defined(X)" for some
reason, please e-mail Chromium-dev about it.  The most recent advice I received
was to always prefer "#if defined(X)" for everything.

[Nico, 2014-03-13 20:23:47]

On Thu, Mar 13, 2014 at 1:21 PM, <pkasting@chromium.org> wrote:

> FWIW, if you guys think "#if X" is preferable to "#if defined(X)" for some
> reason, please e-mail Chromium-dev about it.  The most recent advice I
> received
> was to always prefer "#if defined(X)" for everything.
>

That's still the case, but this macro is used in ifs in a few places, which
isn't possible with an defined() style macro.


>
> https://codereview.chromium.org/195973002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-13 20:40:05]

On Thu, Mar 13, 2014 at 4:23 PM, Nico Weber <thakis@chromium.org> wrote:

> On Thu, Mar 13, 2014 at 1:21 PM, <pkasting@chromium.org> wrote:
>
>> FWIW, if you guys think "#if X" is preferable to "#if defined(X)" for some
>> reason, please e-mail Chromium-dev about it.  The most recent advice I
>> received
>> was to always prefer "#if defined(X)" for everything.
>>
>
> That's still the case, but this macro is used in ifs in a few places,
> which isn't possible with an defined() style macro.
>

It's technically possible of course to rewrite them such that we use #ifdef
instead, and I would prefer that, but I don't wanna cause friction cuz
overall I'm REALLY happy to have something I can use in the preprocessor at
all.


>
>
>>
>> https://codereview.chromium.org/195973002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Xianzhu, 2014-03-13 21:11:36]

On 2014/03/13 20:40:05, danakj wrote:
> On Thu, Mar 13, 2014 at 4:23 PM, Nico Weber <mailto:thakis@chromium.org>
wrote:
> 
> > On Thu, Mar 13, 2014 at 1:21 PM, <mailto:pkasting@chromium.org> wrote:
> >
> >> FWIW, if you guys think "#if X" is preferable to "#if defined(X)" for some
> >> reason, please e-mail Chromium-dev about it.  The most recent advice I
> >> received
> >> was to always prefer "#if defined(X)" for everything.
> >>
> >
> > That's still the case, but this macro is used in ifs in a few places,
> > which isn't possible with an defined() style macro.
> >
> 
> It's technically possible of course to rewrite them such that we use #ifdef
> instead, and I would prefer that, but I don't wanna cause friction cuz
> overall I'm REALLY happy to have something I can use in the preprocessor at
> all.
> 

+akalin

I would also prefer that, after making the following clear:

DCHECK has a special behavior (both documented in logging.h and tested in
logging_unittest.cc): it references all the variables in the condition even if
DCHECK is not enabled. I think the original if(DCHECK_IS_ON()) usage is also to
keep this behavior.

I'm wondering if the behavior was designed just to make the following code
compile without warnings with shortest code:
  int some_var = ...;
  DCHECK(condition(some_var))

I think we could change the above to:
#if defined(ENABLE_DCHECK)
  int foo = ...;
  DCHECK(condition(foo));
#endif
with two more lines, without requiring the special behavior of DCHECK.

wdyt?

[Nico, 2014-03-13 21:17:23]

(akalin left chrome-team a few weeks ago.)

On Thu, Mar 13, 2014 at 2:11 PM, <wangxianzhu@chromium.org> wrote:

> On 2014/03/13 20:40:05, danakj wrote:
>
>> On Thu, Mar 13, 2014 at 4:23 PM, Nico Weber <mailto:thakis@chromium.org>
>>
> wrote:
>
>
>  > On Thu, Mar 13, 2014 at 1:21 PM, <mailto:pkasting@chromium.org> wrote:
>> >
>> >> FWIW, if you guys think "#if X" is preferable to "#if defined(X)" for
>> some
>> >> reason, please e-mail Chromium-dev about it.  The most recent advice I
>> >> received
>> >> was to always prefer "#if defined(X)" for everything.
>> >>
>> >
>> > That's still the case, but this macro is used in ifs in a few places,
>> > which isn't possible with an defined() style macro.
>> >
>>
>
>  It's technically possible of course to rewrite them such that we use
>> #ifdef
>> instead, and I would prefer that, but I don't wanna cause friction cuz
>> overall I'm REALLY happy to have something I can use in the preprocessor
>> at
>> all.
>>
>
>
> +akalin
>
> I would also prefer that, after making the following clear:
>
> DCHECK has a special behavior (both documented in logging.h and tested in
> logging_unittest.cc): it references all the variables in the condition
> even if
> DCHECK is not enabled. I think the original if(DCHECK_IS_ON()) usage is
> also to
> keep this behavior.
>
> I'm wondering if the behavior was designed just to make the following code
> compile without warnings with shortest code:
>   int some_var = ...;
>   DCHECK(condition(some_var))
>
> I think we could change the above to:
> #if defined(ENABLE_DCHECK)
>   int foo = ...;
>   DCHECK(condition(foo));
> #endif
> with two more lines, without requiring the special behavior of DCHECK.
>
> wdyt?
>

I think we should try to keep the current behavior.


>
>
>
> https://codereview.chromium.org/195973002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-03-13 21:17:55]

On 2014/03/13 21:11:36, Xianzhu wrote:
> I think we could change the above to:
> #if defined(ENABLE_DCHECK)
>   int foo = ...;
>   DCHECK(condition(foo));
> #endif
> with two more lines, without requiring the special behavior of DCHECK.

Ugh.  I would hate to have rare conditions where, occasionally, you have to
guard DCHECK with a test for ENABLE_DCHECK.  That seems much worse than anything
that's happening here.

[Xianzhu, 2014-03-13 22:06:04]

On 2014/03/13 21:17:55, Peter Kasting wrote:
> On 2014/03/13 21:11:36, Xianzhu wrote:
> > I think we could change the above to:
> > #if defined(ENABLE_DCHECK)
> >   int foo = ...;
> >   DCHECK(condition(foo));
> > #endif
> > with two more lines, without requiring the special behavior of DCHECK.
> 
> Ugh.  I would hate to have rare conditions where, occasionally, you have to
> guard DCHECK with a test for ENABLE_DCHECK.  That seems much worse than
anything
> that's happening here.

Consider the following cases:
- The variable used only in DCHECK needs complex computation;
- Some class data member is used only in DCHECK.

For these cases, the current code guard both the definition/computation of the
variable/member and the DCHECK with "#ifndef DEBUG" (because we didn't have a
compile-time macro to test if DCHECK is enabled before we made DCHECK on/off at
compile-time only). Now we may want to change some of the guards to "#ifdef
ENABLE_DCHECK" to enable them when dcheck_always_on=1, then we will have to
guard DCHECKs with ENABLE_DCHECK.

If we change DCHECK's always-reference behavior to reference-only-if-enabled:
- we can avoid the guards for DCHECKs in the above cases;
- but have to guard definitions of local variables used only in DCHECKs.

What's the best trade-off?

[Nico, 2014-03-13 22:57:43]

On 2014/03/13 22:06:04, Xianzhu wrote:
> On 2014/03/13 21:17:55, Peter Kasting wrote:
> > On 2014/03/13 21:11:36, Xianzhu wrote:
> > > I think we could change the above to:
> > > #if defined(ENABLE_DCHECK)
> > >   int foo = ...;
> > >   DCHECK(condition(foo));
> > > #endif
> > > with two more lines, without requiring the special behavior of DCHECK.
> > 
> > Ugh.  I would hate to have rare conditions where, occasionally, you have to
> > guard DCHECK with a test for ENABLE_DCHECK.  That seems much worse than
> anything
> > that's happening here.
> 
> Consider the following cases:
> - The variable used only in DCHECK needs complex computation;
> - Some class data member is used only in DCHECK.
> 
> For these cases, the current code guard both the definition/computation of the
> variable/member and the DCHECK with "#ifndef DEBUG" (because we didn't have a
> compile-time macro to test if DCHECK is enabled before we made DCHECK on/off
at
> compile-time only). Now we may want to change some of the guards to "#ifdef
> ENABLE_DCHECK" to enable them when dcheck_always_on=1, then we will have to
> guard DCHECKs with ENABLE_DCHECK.
> 
> If we change DCHECK's always-reference behavior to reference-only-if-enabled:
> - we can avoid the guards for DCHECKs in the above cases;
> - but have to guard definitions of local variables used only in DCHECKs.
> 
> What's the best trade-off?

I understood Peter's second message as "please ignore my first message then".
Maybe that's not what he meant, though. (I think this CL is good as is)

[Peter Kasting, 2014-03-13 23:04:13]

On 2014/03/13 22:57:43, Nico wrote:
> I understood Peter's second message as "please ignore my first message then".
> Maybe that's not what he meant, though. (I think this CL is good as is)

The only thing my first message said is that I was surprised if we ever
preferred "#if X" to "#if defined(X)" in a situation where both would work.

My second message, independent of the first, said we shouldn't rewrite DCHECK()
such that code that works today without any guards at all starts needing #if
ENABLE_DCHECK (or similar) around DCHECKs.

[Xianzhu, 2014-03-14 00:00:53]

On 2014/03/13 23:04:13, Peter Kasting wrote:
> On 2014/03/13 22:57:43, Nico wrote:
> > I understood Peter's second message as "please ignore my first message
then".
> > Maybe that's not what he meant, though. (I think this CL is good as is)
> 
> The only thing my first message said is that I was surprised if we ever
> preferred "#if X" to "#if defined(X)" in a situation where both would work.
> 
> My second message, independent of the first, said we shouldn't rewrite
DCHECK()
> such that code that works today without any guards at all starts needing #if
> ENABLE_DCHECK (or similar) around DCHECKs.

However, without rewriting DCHECK, we'll need guards around DCHECKs in the cases
in #23.
Actually rewriting DCHECK doesn't require guards around DCHECK, but around the
code that is only for DCHECK. I should have written the example as below:

#if defined(ENABLE_DCHECK)
  int foo = ...;
#endif
  DCHECK(condition(foo));

I'm CQ'ing anyway :)

[Xianzhu, 2014-03-14 00:01:04]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-03-14 00:02:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/195973002/80001

[commit-bot: I haz the power, 2014-03-14 00:28:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/195973002/80001

[commit-bot: I haz the power, 2014-03-14 02:23:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 02:23:10]

Retried try job too often on linux_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[danakj, 2014-03-14 17:09:43]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/195973002/80001

[commit-bot: I haz the power, 2014-03-14 18:39:57]

Change committed as 257156