cc: Switch to Chromium DCHECKs and LOGs

cc: Switch to Chromium DCHECKs LOGs

We can't compile-guard code and use DCHECK since it can be enabled at runtime.

This removes all compile-time guards, and makes use of DCHECK instead of ASSERT. We use DCHECK_IS_ON() to early out and avoid extra work just for DCHECK where possible as well.

This also replaces use of LOG_ERROR("Foo") with LOG(ERROR)<<"Foo";

R=enne,jamesr

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162296

[enne (OOO), 2012-10-12 01:31:51]

lgtm

[commit-bot: I haz the power, 2012-10-12 01:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11048044/1

[commit-bot: I haz the power, 2012-10-12 01:46:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[danakj, 2012-10-12 21:47:28]

PTAL I updated the description, this replaces all asserts with chromium-style
ones.

[danakj, 2012-10-12 21:48:50]

@skaslev FYI on DCHECK_IS_ENABLED() for renderer clearing.

[slavi, 2012-10-12 22:10:13]

https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h
File cc/CCCompletionEvent.h (left):

https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#ol...
cc/CCCompletionEvent.h:53: #ifndef NDEBUG
Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of removing
them?

On the other hand this class should probably be removed altogether and just use
base::WaitableEvent instead.

But the point still stands for all the other removed #ifndef NDEBUG in this CL.

[danakj, 2012-10-12 22:13:20]

https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h
File cc/CCCompletionEvent.h (left):

https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#ol...
cc/CCCompletionEvent.h:53: #ifndef NDEBUG
On 2012/10/12 22:10:13, slaf wrote:
> Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of removing
> them?

ENABLE_DCHECK only exists inside base/logging.h There is currently no
compile-time check to see if DCHECK is on, because there is --enable-dcheck
command line flag.

I am going to propose we remove that. And I think in the meantime I will make a
#define SOMETHING 1 and guard these on that. So that we can replace that guard
with DCHECKS_ALWAYS_ON or something alike it in the future.

[piman, 2012-10-12 22:17:29]

On Fri, Oct 12, 2012 at 3:13 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/11048044/diff/12001/cc/**
>
CCCompletionEvent.h<https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h>
> File cc/CCCompletionEvent.h (left):
>
> https://codereview.chromium.**org/11048044/diff/12001/cc/**
>
CCCompletionEvent.h#oldcode53<https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#oldcode53>
> cc/CCCompletionEvent.h:53: #ifndef NDEBUG
> On 2012/10/12 22:10:13, slaf wrote:
>
>> Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of
>>
> removing
>
>> them?
>>
>
> ENABLE_DCHECK only exists inside base/logging.h There is currently no
> compile-time check to see if DCHECK is on, because there is
> --enable-dcheck command line flag.
>

Note: official builds remove DCHECKs at compile time. Not sure there'd be a
large win here though.

Antoine


>
> I am going to propose we remove that. And I think in the meantime I will
> make a #define SOMETHING 1 and guard these on that. So that we can
> replace that guard with DCHECKS_ALWAYS_ON or something alike it in the
> future.
>
>
https://codereview.chromium.**org/11048044/<https://codereview.chromium.org/1...
>
> --
>
>
>

[danakj, 2012-10-12 22:18:55]

On Fri, Oct 12, 2012 at 3:17 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Fri, Oct 12, 2012 at 3:13 PM, <danakj@chromium.org> wrote:
>>
>>
>> https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h
>> File cc/CCCompletionEvent.h (left):
>>
>>
>>
https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#ol...
>> cc/CCCompletionEvent.h:53: #ifndef NDEBUG
>> On 2012/10/12 22:10:13, slaf wrote:
>>>
>>> Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of
>>
>> removing
>>>
>>> them?
>>
>>
>> ENABLE_DCHECK only exists inside base/logging.h There is currently no
>> compile-time check to see if DCHECK is on, because there is
>> --enable-dcheck command line flag.
>
>
> Note: official builds remove DCHECKs at compile time. Not sure there'd be a
> large win here though.

Yes but duplicating the guards for official build checks out of
base/logging isn't a great idea. We're just going to not support
--enable-dchecks while I propose we do away with it to chromium-dev.

[slavi, 2012-10-12 22:28:58]

On 2012/10/12 22:18:55, danakj wrote:
> On Fri, Oct 12, 2012 at 3:17 PM, Antoine Labour <mailto:piman@chromium.org>
wrote:
> >
> >
> > On Fri, Oct 12, 2012 at 3:13 PM, <mailto:danakj@chromium.org> wrote:
> >>
> >>
> >> https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h
> >> File cc/CCCompletionEvent.h (left):
> >>
> >>
> >>
>
https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#ol...
> >> cc/CCCompletionEvent.h:53: #ifndef NDEBUG
> >> On 2012/10/12 22:10:13, slaf wrote:
> >>>
> >>> Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of
> >>
> >> removing
> >>>
> >>> them?
> >>
> >>
> >> ENABLE_DCHECK only exists inside base/logging.h There is currently no
> >> compile-time check to see if DCHECK is on, because there is
> >> --enable-dcheck command line flag.
> >
> >
> > Note: official builds remove DCHECKs at compile time. Not sure there'd be a
> > large win here though.
> 
> Yes but duplicating the guards for official build checks out of
> base/logging isn't a great idea. We're just going to not support
> --enable-dchecks while I propose we do away with it to chromium-dev.

I agree that the command line DCHECK flag doesn't sound like a great idea.
Google's DCHECK is decided compile time by the NDEBUG define.

Is the plan to revisit all the removed #if NDEBUGs after we remove the compile
time DCHEKs?

[slavi, 2012-10-12 22:29:52]

On 2012/10/12 22:28:58, slaf wrote:
> On 2012/10/12 22:18:55, danakj wrote:
> > On Fri, Oct 12, 2012 at 3:17 PM, Antoine Labour <mailto:piman@chromium.org>
> wrote:
> > >
> > >
> > > On Fri, Oct 12, 2012 at 3:13 PM, <mailto:danakj@chromium.org> wrote:
> > >>
> > >>
> > >>
https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h
> > >> File cc/CCCompletionEvent.h (left):
> > >>
> > >>
> > >>
> >
>
https://codereview.chromium.org/11048044/diff/12001/cc/CCCompletionEvent.h#ol...
> > >> cc/CCCompletionEvent.h:53: #ifndef NDEBUG
> > >> On 2012/10/12 22:10:13, slaf wrote:
> > >>>
> > >>> Maybe change all the #ifndef NDEBUG to #if ENABLE_DCHECK instead of
> > >>
> > >> removing
> > >>>
> > >>> them?
> > >>
> > >>
> > >> ENABLE_DCHECK only exists inside base/logging.h There is currently no
> > >> compile-time check to see if DCHECK is on, because there is
> > >> --enable-dcheck command line flag.
> > >
> > >
> > > Note: official builds remove DCHECKs at compile time. Not sure there'd be
a
> > > large win here though.
> > 
> > Yes but duplicating the guards for official build checks out of
> > base/logging isn't a great idea. We're just going to not support
> > --enable-dchecks while I propose we do away with it to chromium-dev.
> 
> I agree that the command line DCHECK flag doesn't sound like a great idea.
> Google's DCHECK is decided compile time by the NDEBUG define.
> 
> Is the plan to revisit all the removed #if NDEBUGs after we remove the compile
> time DCHEKs?

*Is the plan to revisit all the removed #if NDEBUGs after we remove the DCHECK
runtime flag?

[danakj, 2012-10-12 22:31:39]

On 2012/10/12 22:29:52, slaf wrote:
> *Is the plan to revisit all the removed #if NDEBUGs after we remove the DCHECK
> runtime flag?

I'm actually remaking this CL to put compile flags back in. We can just not
support --enable-dchecks. This is similar to base/threading for reference.

In the future if the runtime flag goes away and we get a define to use in base/
we can make use of that instead of using our own.

[enne (OOO), 2012-10-15 18:23:34]

Can you add a presubmit check to prevent the use of a raw DCHECK in cc?

[slavi, 2012-10-16 00:58:45]

On 2012/10/15 18:23:34, enne wrote:
> Can you add a presubmit check to prevent the use of a raw DCHECK in cc?

I'm sorry if this is getting nagging but don't quite agree with the need of
CC_DCHECK. I believe that it's unnecessary.

There are two kind of DCHECKs -- normal ones and the ones that rely on data
behind a #ifdef NDEBUG. We can leave the normal ones as DCHECKs and just hide
the other ones behind #ifdef NDEBUG.

See
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...
and
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...
for an example.

I don't think we need CC_DCHECK_ENABLED and CC_DCHECK when we can get by with
NDEBUG and DCHECK as the rest of Chrome does.

[slavi, 2012-10-16 00:59:59]

On 2012/10/16 00:58:45, slavi wrote:
> On 2012/10/15 18:23:34, enne wrote:
> > Can you add a presubmit check to prevent the use of a raw DCHECK in cc?
> 
> I'm sorry if this is getting nagging but don't quite agree with the need of
> CC_DCHECK. I believe that it's unnecessary.
> 
> There are two kind of DCHECKs -- normal ones and the ones that rely on data
> behind a #ifdef NDEBUG. We can leave the normal ones as DCHECKs and just hide
> the other ones behind #ifdef NDEBUG.
> 
> See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...
> and
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...
> for an example.
> 
> I don't think we need CC_DCHECK_ENABLED and CC_DCHECK when we can get by with
> NDEBUG and DCHECK as the rest of Chrome does.

s/#ifdef NDEBUG/#ifndef NDEBUG/g

[danakj, 2012-10-16 01:03:26]

NDEBUG is not enough, you need !NDEBUG || DCHECK_ALWAYS_ON.

Try bots are release but with dcheck on, and we want these dchecks to run on try
bots. We could, indeed, guard all these DCHECK uses with !NDEBUG ||
DCHECK_ALWAYS_ON, but that's difficult for new code to get right compared to
CC_DCHECK().

I am still going to work at making it possible for us to just use DCHECK
everywhere (make its presence compile-time checkable instead of runtime).

[slavi, 2012-10-16 06:23:09]

lgtm

http://codereview.chromium.org/11048044/diff/17002/cc/yuv_video_draw_quad.cc
File cc/yuv_video_draw_quad.cc (right):

http://codereview.chromium.org/11048044/diff/17002/cc/yuv_video_draw_quad.cc#...
cc/yuv_video_draw_quad.cc:28: CC_DCHECK(quad->material() ==
CCDrawQuad::YUVVideoContent);
I would say that this ASSERT should be a DCHECK rather than a CC_CHECK.

[danakj, 2012-10-16 18:12:39]

presubmit test added

[enne (OOO), 2012-10-16 18:34:22]

https://codereview.chromium.org/11048044/diff/28001/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/11048044/diff/28001/cc/PRESUBMIT.py#newcode14
cc/PRESUBMIT.py:14: white_list = tuple(white_list or ('^cc/.*\.(cc|h)$',))
nit: r'^cc...

Also, what about just sticking these as the default args, e.g. white_list =
CC_SOURCE_FILES, black_list=input_api.DEFAULT_BLACK_LIST?

https://codereview.chromium.org/11048044/diff/28001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/11048044/diff/28001/cc/cc.gyp#newcode8
cc/cc.gyp:8: 'dcheck.h',
Where is this file?

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
File cc/resource_provider_unittest.cc (right):

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
cc/resource_provider_unittest.cc:64: CC_DCHECK(mailbox && mailbox <
m_nextMailBox);
This is in a unit test, so maybe ASSERT and ASSERT_LT instead of DCHECK?

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
cc/resource_provider_unittest.cc:74: CC_DCHECK(mailbox && mailbox <
m_nextMailBox);
Same here?

[slavi, 2012-10-16 19:05:04]

On 2012/10/16 18:34:43, enne wrote:

Why are we forbidding DCHECKs in cc? 

All of Chrome is using just raw DCHECKS. Is there a chromium-style group where
we can have a broader discussion? 

Or bring a reviewer that is more experienced with Chrome's style?

[danakj, 2012-10-16 19:11:51]

ptal no more missing files :)

https://codereview.chromium.org/11048044/diff/28001/cc/PRESUBMIT.py
File cc/PRESUBMIT.py (right):

https://codereview.chromium.org/11048044/diff/28001/cc/PRESUBMIT.py#newcode14
cc/PRESUBMIT.py:14: white_list = tuple(white_list or ('^cc/.*\.(cc|h)$',))
On 2012/10/16 18:34:22, enne wrote:
> nit: r'^cc...
> 
> Also, what about just sticking these as the default args, e.g. white_list =
> CC_SOURCE_FILES, black_list=input_api.DEFAULT_BLACK_LIST?

Aded CC_SOURCE_FILES. The black_list has to stay the same, can't refer to other
parameters when trying to set a default parameter value.

https://codereview.chromium.org/11048044/diff/28001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/11048044/diff/28001/cc/cc.gyp#newcode8
cc/cc.gyp:8: 'dcheck.h',
On 2012/10/16 18:34:22, enne wrote:
> Where is this file?

Somehow escaped adding, added now.

Also the config.h got missed entirely when I pushed the changes back home.
Recreated that and added it.

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
File cc/resource_provider_unittest.cc (right):

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
cc/resource_provider_unittest.cc:64: CC_DCHECK(mailbox && mailbox <
m_nextMailBox);
On 2012/10/16 18:34:22, enne wrote:
> This is in a unit test, so maybe ASSERT and ASSERT_LT instead of DCHECK?

Done.

https://codereview.chromium.org/11048044/diff/28001/cc/resource_provider_unit...
cc/resource_provider_unittest.cc:74: CC_DCHECK(mailbox && mailbox <
m_nextMailBox);
On 2012/10/16 18:34:22, enne wrote:
> Same here?

This one has to be a dcheck because the function has a non-void return type.
ASSERT_TRUE tries to return.

[danakj, 2012-10-16 19:17:37]

On 2012/10/16 19:05:04, slavi wrote:
> On 2012/10/16 18:34:43, enne wrote:
> 
> Why are we forbidding DCHECKs in cc? 

Just to make the code consistent and not a mix of DCHECK and CC_DCHECK beside
each other.

> All of Chrome is using just raw DCHECKS. Is there a chromium-style group where
> we can have a broader discussion? 

I had a discussion in #chromium last week about this. Wrapping DCHECK isn't
desirable, but we need to use guards at the moment.

Having a mix of DCHECKS in the cc/ code gets more confusing IMO than having just
one with a presubmit message about it. You will end up with

DCHECK(something)
CC_DCHECK(some other thing)

And it's not clear what the difference between these two is. When we can start
compile-checking for DCHECK then we can search-replace all CC_DCHECKS over. In
the meantime we have a choice between mixing or not, IMHO mixing is less
desirable.

[slavi, 2012-10-16 19:22:34]

On 2012/10/16 19:17:37, danakj wrote:
> On 2012/10/16 19:05:04, slavi wrote:
> > On 2012/10/16 18:34:43, enne wrote:
> > 
> > Why are we forbidding DCHECKs in cc? 
> 
> Just to make the code consistent and not a mix of DCHECK and CC_DCHECK beside
> each other.
> 
> > All of Chrome is using just raw DCHECKS. Is there a chromium-style group
where
> > we can have a broader discussion? 
> 
> I had a discussion in #chromium last week about this. Wrapping DCHECK isn't
> desirable, but we need to use guards at the moment.
> 
> Having a mix of DCHECKS in the cc/ code gets more confusing IMO than having
just
> one with a presubmit message about it. You will end up with
> 
> DCHECK(something)
> CC_DCHECK(some other thing)
> 
> And it's not clear what the difference between these two is. When we can start
> compile-checking for DCHECK then we can search-replace all CC_DCHECKS over. In
> the meantime we have a choice between mixing or not, IMHO mixing is less
> desirable.

May I suggest that we use DCHECKs by default, like all of Chrome, and use
CC_DCHECK only when CC_DCHECK behavior is really necessary?

[enne (OOO), 2012-10-16 19:27:54]

On 2012/10/16 19:22:34, slavi wrote:
> 
> May I suggest that we use DCHECKs by default, like all of Chrome, and use
> CC_DCHECK only when CC_DCHECK behavior is really necessary?

My concern is that cc (maybe unlike most of Chromium) has a lot of conditionally
compiled debug variables.

If we mix DCHECK AND CC_DCHECK, then somebody might mix DCHECK with
CC_DCHECK_ENABLED, which might not even be defined because dcheck.h wasn't
included.

Mostly, I feel like a presubmit rule to always use CC_DCHECK when in cc/ is a
clearer rule to explain/remember/get right then "sometimes use CC_DCHECK, but
only when asserting on variables that are meaningful #if CC_DCHECK_ENABLED.

[slavi, 2012-10-16 19:33:18]

On 2012/10/16 19:27:54, enne wrote:
> On 2012/10/16 19:22:34, slavi wrote:
> > 
> > May I suggest that we use DCHECKs by default, like all of Chrome, and use
> > CC_DCHECK only when CC_DCHECK behavior is really necessary?
> 
> My concern is that cc (maybe unlike most of Chromium) has a lot of
conditionally
> compiled debug variables.
> 
> If we mix DCHECK AND CC_DCHECK, then somebody might mix DCHECK with
> CC_DCHECK_ENABLED, which might not even be defined because dcheck.h wasn't
> included.
> 
> Mostly, I feel like a presubmit rule to always use CC_DCHECK when in cc/ is a
> clearer rule to explain/remember/get right then "sometimes use CC_DCHECK, but
> only when asserting on variables that are meaningful #if CC_DCHECK_ENABLED.

Chrome does use a lot of debug vars too. 

The proper rule in the rest of the code is always use DCHECK unless you debug
variables are behind #ifndef NDEBUG. Then put you DCHECK inside of #ifndef
NDEBUG. See
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...

[enne (OOO), 2012-10-16 19:35:47]

On 2012/10/16 19:33:18, slavi wrote:

> The proper rule in the rest of the code is always use DCHECK unless you debug
> variables are behind #ifndef NDEBUG. Then put you DCHECK inside of #ifndef
> NDEBUG. See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...

...and in my opinion it's broken to only use NDEBUG.  You can have dchecks in
release builds.  That's why dcheck.h says "#if (!defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON))".

[enne (OOO), 2012-10-16 19:36:18]

Or, I want cc to have dchecks on release bots even when they involve member
variables.

[danakj, 2012-10-16 19:38:14]

On 2012/10/16 19:33:18, slavi wrote:
> Chrome does use a lot of debug vars too. 
> 
> The proper rule in the rest of the code is always use DCHECK unless you debug
> variables are behind #ifndef NDEBUG. Then put you DCHECK inside of #ifndef
> NDEBUG. See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/cance...

The example given to me in #chromium of what we want is here

http://code.google.com/searchframe#OAMlx_jo-ck/src/base/threading/thread_chec...

[slavi, 2012-10-16 19:39:09]

On 2012/10/16 19:36:18, enne wrote:
> Or, I want cc to have dchecks on release bots even when they involve member
> variables.

You don't have to put you DCHECKs under #ifndef NDEBUG. Only when you're
checking on data that's under #ifndef NDEBUG. Just a DCHECK without #ifndef is
perfectly valid also.  You can also use CHECK if you want to check always.

[enne (OOO), 2012-10-16 19:47:05]

My chain of reasoning goes like this:

* I want all DCHECKS in cc on in release builds
* I don't want to have to have everybody copy and paste "#if (!defined(NDEBUG)
||
defined(DCHECK_ALWAYS_ON))" everywhere
* Therefore we add a CC_DCHECK_ENABLED macro to do it
* I also don't want folks to accidentally #if CC_DCHECK_ENABLED without dcheck.h
being included (making those dchecks "always off")
* I also don't want it to be easy to DCHECK something that's #if NDEBUG'd,
causing compile failures in release builds with DCHECKs
* I also want it to be easy to remember what to use when when reviewing and
submitting cc code

Therefore, CC_DCHECK always instead of DCHECK and a CC_DCHECK_ENABLED macro to
guard anything we CC_DCHECK.  This seems easy to get right.

Dana is going to try to push to get compile-time flag for dchecks in Chromium,
and when/if that goes through, we can replace all these.  This is likely just a
temporary situation.

[slavi, 2012-10-16 19:57:56]

On 2012/10/16 19:47:05, enne wrote:
> My chain of reasoning goes like this:
> 
> * I want all DCHECKS in cc on in release builds

DCHECK does that.

> * I don't want to have to have everybody copy and paste "#if (!defined(NDEBUG)
> ||
> defined(DCHECK_ALWAYS_ON))" everywhere

You don't need that everywhere. You only need a #ifdef for you DCHECK if you
data is behind an #ifdef. The #ifdef you use for your DCHECK should be the same
as the #ifdef the data you're using is behind.

> * Therefore we add a CC_DCHECK_ENABLED macro to do it

See previous point. CC_DCHECK_ENABLED is unnecessary.

> * I also don't want folks to accidentally #if CC_DCHECK_ENABLED without
dcheck.h
> being included (making those dchecks "always off")
> * I also don't want it to be easy to DCHECK something that's #if NDEBUG'd,
> causing compile failures in release builds with DCHECKs
> * I also want it to be easy to remember what to use when when reviewing and
> submitting cc code
> 
> Therefore, CC_DCHECK always instead of DCHECK and a CC_DCHECK_ENABLED macro to
> guard anything we CC_DCHECK.  This seems easy to get right.
> 
> Dana is going to try to push to get compile-time flag for dchecks in Chromium,
> and when/if that goes through, we can replace all these.  This is likely just
a
> temporary situation.


Anyway, if this is temporary change, should be all right.

[enne (OOO), 2012-10-16 20:01:29]

lgtm

[danakj, 2012-10-16 20:01:30]

piman had a good suggestion to use the official build macros.

#define CC_DCHECK_ENABLED() !LOGGING_IS_OFFICIAL_BUILD

Then we can guard variables with CC_DCHECK_ENABLED(). And we can just use DCHECK
everywhere.

[slavi, 2012-10-16 20:02:44]

On 2012/10/16 20:01:30, danakj wrote:
> piman had a good suggestion to use the official build macros.
> 
> #define CC_DCHECK_ENABLED() !LOGGING_IS_OFFICIAL_BUILD
> 
> Then we can guard variables with CC_DCHECK_ENABLED(). And we can just use
DCHECK
> everywhere.

Sounds awesome =-)

[commit-bot: I haz the power, 2012-10-16 20:27:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11048044/33004

[slavi, 2012-10-16 20:31:46]

https://codereview.chromium.org/11048044/diff/33004/cc/gl_renderer_unittest.cc
File cc/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/11048044/diff/33004/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:254: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:263: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:406: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc
File cc/layer_impl.cc (left):

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc#oldcode69
cc/layer_impl.cc:69: #ifndef NDEBUG
Keep this #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc#newcode58
cc/layer_impl.cc:58: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc#newcode129
cc/layer_impl.cc:129: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.cc#newcode138
cc/layer_impl.cc:138: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11048044/diff/33004/cc/layer_impl.h#newcode371
cc/layer_impl.h:371: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.cc
File cc/prioritized_texture.cc (left):

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.cc...
cc/prioritized_texture.cc:151: #ifndef NDEBUG
Keep this #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.cc
File cc/prioritized_texture.cc (right):

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.cc...
cc/prioritized_texture.cc:135: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.cc...
cc/prioritized_texture.cc:151: DCHECK(resourceProvider == m_resourceProvider);
Keep this #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.h
File cc/prioritized_texture.h (right):

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.h#...
cc/prioritized_texture.h:115: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/program_binding.cc
File cc/program_binding.cc (right):

https://codereview.chromium.org/11048044/diff/33004/cc/program_binding.cc#new...
cc/program_binding.cc:69: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

https://codereview.chromium.org/11048044/diff/33004/cc/program_binding.cc#new...
cc/program_binding.cc:100: #if CC_DCHECK_ENABLED()
Keep this as #ifndef NDEBUG?

[danakj, 2012-10-16 20:36:17]

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.h
File cc/prioritized_texture.h (right):

https://codereview.chromium.org/11048044/diff/33004/cc/prioritized_texture.h#...
cc/prioritized_texture.h:115: #if CC_DCHECK_ENABLED()
On 2012/10/16 20:31:46, slavi wrote:
> Keep this as #ifndef NDEBUG?

No, we want debug checks on the trybots..

[commit-bot: I haz the power, 2012-10-16 20:49:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11048044/41001

[commit-bot: I haz the power, 2012-10-16 22:51:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11048044/26107

[commit-bot: I haz the power, 2012-10-17 00:50:22]

Change committed as 162296

[danakj, 2012-10-17 15:37:38]

Sad news. This got reverted for breaking the build.

Apparently you can have DCHECKS enabled in an official build. So guarding on
LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome...

[piman, 2012-10-17 16:34:52]

On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:

> Sad news. This got reverted for breaking the build.
>
> Apparently you can have DCHECKS enabled in an official build. So guarding
> on
> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>

It looks like DCHECK, by design, will still reference the variables in the
condition when compiled out.
Still, doesn't mean we need to fork the style guide.

Most of the places a DCHECK will be fine. In a few places where we want to
avoid the cost of keeping track of some variables, we can #ifdef it on some
condition, and #ifdef the corresponding DCHECK(s), on the same condition.
But limit it to those places.

It doesn't have to affect the 95% of other DCHECKs.


>
> http://build.chromium.org/p/**chromium.chrome/builders/**
> Google%20Chrome%20ChromeOS/**builds/43608/steps/compile/**
>
logs/stdio#error1<http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/43608/steps/compile/logs/stdio#error1>
>
>
https://codereview.chromium.**org/11048044/<https://codereview.chromium.org/1...
>

[danakj, 2012-10-17 16:38:59]

On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
>>
>> Sad news. This got reverted for breaking the build.
>>
>> Apparently you can have DCHECKS enabled in an official build. So guarding
>> on
>> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>
>
> It looks like DCHECK, by design, will still reference the variables in the
> condition when compiled out.
> Still, doesn't mean we need to fork the style guide.
>
> Most of the places a DCHECK will be fine. In a few places where we want to
> avoid the cost of keeping track of some variables, we can #ifdef it on some
> condition, and #ifdef the corresponding DCHECK(s), on the same condition.
> But limit it to those places.
>
> It doesn't have to affect the 95% of other DCHECKs.

The problem that I see with this is that then some DCHECKs are run in
different build conditions than others. It makes it trickier to know
what's being tested. If you get a crash dump with a DCHECK failing,
and there are guarded DCHECKs above it, did they pass or were they
compiled out?

>>
>>
>>
>>
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome...
>>
>> https://codereview.chromium.org/11048044/
>
>

[piman, 2012-10-17 16:45:24]

On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
> wrote:
> >
> >
> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
> >>
> >> Sad news. This got reverted for breaking the build.
> >>
> >> Apparently you can have DCHECKS enabled in an official build. So
> guarding
> >> on
> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
> >
> >
> > It looks like DCHECK, by design, will still reference the variables in
> the
> > condition when compiled out.
> > Still, doesn't mean we need to fork the style guide.
> >
> > Most of the places a DCHECK will be fine. In a few places where we want
> to
> > avoid the cost of keeping track of some variables, we can #ifdef it on
> some
> > condition, and #ifdef the corresponding DCHECK(s), on the same condition.
> > But limit it to those places.
> >
> > It doesn't have to affect the 95% of other DCHECKs.
>
> The problem that I see with this is that then some DCHECKs are run in
> different build conditions than others. It makes it trickier to know
> what's being tested. If you get a crash dump with a DCHECK failing,
> and there are guarded DCHECKs above it, did they pass or were they
> compiled out?
>

Then make it a CHECK?


>
> >>
> >>
> >>
> >>
>
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome...
> >>
> >> https://codereview.chromium.org/11048044/
> >
> >
>

[danakj, 2012-10-17 16:48:08]

On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org> wrote:
>>
>> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
>> wrote:
>> >
>> >
>> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
>> >>
>> >> Sad news. This got reverted for breaking the build.
>> >>
>> >> Apparently you can have DCHECKS enabled in an official build. So
>> >> guarding
>> >> on
>> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>> >
>> >
>> > It looks like DCHECK, by design, will still reference the variables in
>> > the
>> > condition when compiled out.
>> > Still, doesn't mean we need to fork the style guide.
>> >
>> > Most of the places a DCHECK will be fine. In a few places where we want
>> > to
>> > avoid the cost of keeping track of some variables, we can #ifdef it on
>> > some
>> > condition, and #ifdef the corresponding DCHECK(s), on the same
>> > condition.
>> > But limit it to those places.
>> >
>> > It doesn't have to affect the 95% of other DCHECKs.
>>
>> The problem that I see with this is that then some DCHECKs are run in
>> different build conditions than others. It makes it trickier to know
>> what's being tested. If you get a crash dump with a DCHECK failing,
>> and there are guarded DCHECKs above it, did they pass or were they
>> compiled out?
>
>
> Then make it a CHECK?

I'm not sure how that helps. You still don't know if the condition in
the CHECK passed or it was just not compiled in. I think we should
have all our checks enabled/disabled together.

[jamesr1, 2012-10-17 17:02:45]

On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
> wrote:
> >
> >
> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> >>
> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
> >> >>
> >> >> Sad news. This got reverted for breaking the build.
> >> >>
> >> >> Apparently you can have DCHECKS enabled in an official build. So
> >> >> guarding
> >> >> on
> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
> >> >
> >> >
> >> > It looks like DCHECK, by design, will still reference the variables in
> >> > the
> >> > condition when compiled out.
> >> > Still, doesn't mean we need to fork the style guide.
> >> >
> >> > Most of the places a DCHECK will be fine. In a few places where we
> want
> >> > to
> >> > avoid the cost of keeping track of some variables, we can #ifdef it on
> >> > some
> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
> >> > condition.
> >> > But limit it to those places.
> >> >
> >> > It doesn't have to affect the 95% of other DCHECKs.
> >>
> >> The problem that I see with this is that then some DCHECKs are run in
> >> different build conditions than others. It makes it trickier to know
> >> what's being tested. If you get a crash dump with a DCHECK failing,
> >> and there are guarded DCHECKs above it, did they pass or were they
> >> compiled out?
> >
> >
> > Then make it a CHECK?
>
> I'm not sure how that helps. You still don't know if the condition in
> the CHECK passed or it was just not compiled in. I think we should
> have all our checks enabled/disabled together.
>

CHECK()s are always compiled in.  DCHECK()s never evaluate in the wild.
 When would there be confusion?  I haven't seen this be a problem anywhere
else in chrome.

- James

[piman, 2012-10-17 17:07:28]

On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
> wrote:
> >
> >
> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> >>
> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
> >> >>
> >> >> Sad news. This got reverted for breaking the build.
> >> >>
> >> >> Apparently you can have DCHECKS enabled in an official build. So
> >> >> guarding
> >> >> on
> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
> >> >
> >> >
> >> > It looks like DCHECK, by design, will still reference the variables in
> >> > the
> >> > condition when compiled out.
> >> > Still, doesn't mean we need to fork the style guide.
> >> >
> >> > Most of the places a DCHECK will be fine. In a few places where we
> want
> >> > to
> >> > avoid the cost of keeping track of some variables, we can #ifdef it on
> >> > some
> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
> >> > condition.
> >> > But limit it to those places.
> >> >
> >> > It doesn't have to affect the 95% of other DCHECKs.
> >>
> >> The problem that I see with this is that then some DCHECKs are run in
> >> different build conditions than others. It makes it trickier to know
> >> what's being tested. If you get a crash dump with a DCHECK failing,
> >> and there are guarded DCHECKs above it, did they pass or were they
> >> compiled out?
> >
> >
> > Then make it a CHECK?
>
> I'm not sure how that helps. You still don't know if the condition in
> the CHECK passed or it was just not compiled in.


Just make the condition obvious? e.g. NDEBUG?
I think that's entirely orthogonal.


> I think we should
> have all our checks enabled/disabled together.


The same argument could go with cc vs rest-of-chrome.

We're really only talking about a few asserts here, that are complex enough
that they need specific tracking that we don't want to do in official
builds. It's a fairly rare case. I don't think that justifies changing
every assert, subtly changing the semantic, causing additional cognitive
cost for readers of the code, and extra reviews cycles that will constantly
happen because of the style fork.

Antoine



>
> --
>
>
>

[danakj, 2012-10-17 17:07:34]

On Wed, Oct 17, 2012 at 1:02 PM, James Robinson <jamesr@google.com> wrote:
>
>
> On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org> wrote:
>>
>> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
>> wrote:
>> >
>> >
>> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
>> > wrote:
>> >>
>> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
>> >> >>
>> >> >> Sad news. This got reverted for breaking the build.
>> >> >>
>> >> >> Apparently you can have DCHECKS enabled in an official build. So
>> >> >> guarding
>> >> >> on
>> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>> >> >
>> >> >
>> >> > It looks like DCHECK, by design, will still reference the variables
>> >> > in
>> >> > the
>> >> > condition when compiled out.
>> >> > Still, doesn't mean we need to fork the style guide.
>> >> >
>> >> > Most of the places a DCHECK will be fine. In a few places where we
>> >> > want
>> >> > to
>> >> > avoid the cost of keeping track of some variables, we can #ifdef it
>> >> > on
>> >> > some
>> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
>> >> > condition.
>> >> > But limit it to those places.
>> >> >
>> >> > It doesn't have to affect the 95% of other DCHECKs.
>> >>
>> >> The problem that I see with this is that then some DCHECKs are run in
>> >> different build conditions than others. It makes it trickier to know
>> >> what's being tested. If you get a crash dump with a DCHECK failing,
>> >> and there are guarded DCHECKs above it, did they pass or were they
>> >> compiled out?
>> >
>> >
>> > Then make it a CHECK?
>>
>> I'm not sure how that helps. You still don't know if the condition in
>> the CHECK passed or it was just not compiled in. I think we should
>> have all our checks enabled/disabled together.
>
>
> CHECK()s are always compiled in.  DCHECK()s never evaluate in the wild.
> When would there be confusion?  I haven't seen this be a problem anywhere
> else in chrome.

#if SOME_DCHECK_ENABLED_GUARD
DCHECK(sometimes_compiled_variable)
#endif

DCHECK(some_other_variable)

I don't see how making the DCHECK inside the #if into a CHECK helps us
any. We should either have all our dchecks running or none of them
running. Making situations where only some of them are running is
going to be confusing at some point.

[danakj, 2012-10-17 17:10:22]

On Wed, Oct 17, 2012 at 1:07 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org> wrote:
>>
>> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
>> wrote:
>> >
>> >
>> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
>> > wrote:
>> >>
>> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
>> >> >>
>> >> >> Sad news. This got reverted for breaking the build.
>> >> >>
>> >> >> Apparently you can have DCHECKS enabled in an official build. So
>> >> >> guarding
>> >> >> on
>> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>> >> >
>> >> >
>> >> > It looks like DCHECK, by design, will still reference the variables
>> >> > in
>> >> > the
>> >> > condition when compiled out.
>> >> > Still, doesn't mean we need to fork the style guide.
>> >> >
>> >> > Most of the places a DCHECK will be fine. In a few places where we
>> >> > want
>> >> > to
>> >> > avoid the cost of keeping track of some variables, we can #ifdef it
>> >> > on
>> >> > some
>> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
>> >> > condition.
>> >> > But limit it to those places.
>> >> >
>> >> > It doesn't have to affect the 95% of other DCHECKs.
>> >>
>> >> The problem that I see with this is that then some DCHECKs are run in
>> >> different build conditions than others. It makes it trickier to know
>> >> what's being tested. If you get a crash dump with a DCHECK failing,
>> >> and there are guarded DCHECKs above it, did they pass or were they
>> >> compiled out?
>> >
>> >
>> > Then make it a CHECK?
>>
>> I'm not sure how that helps. You still don't know if the condition in
>> the CHECK passed or it was just not compiled in.
>
>
> Just make the condition obvious? e.g. NDEBUG?
> I think that's entirely orthogonal.

NDEBUG doesn't work for try bots :( This is the whole problem..

>>
>> I think we should
>> have all our checks enabled/disabled together.
>
>
> The same argument could go with cc vs rest-of-chrome.

Yes except they can't appear together in the same method, making one
dependent on the other.

> We're really only talking about a few asserts here, that are complex enough
> that they need specific tracking that we don't want to do in official
> builds. It's a fairly rare case. I don't think that justifies changing every
> assert, subtly changing the semantic, causing additional cognitive cost for
> readers of the code, and extra reviews cycles that will constantly happen
> because of the style fork.

Well, FWIW there will be no extra review cycles since we made a
PRESUBMIT. This saves us from having extra review cycles because
someone chose the wrong guard/didn't remember to guard/etc.

> Antoine
>
>
>>
>>
>> --
>>
>>
>
> --
>
>

[jamesr1, 2012-10-17 17:19:34]

On Wed, Oct 17, 2012 at 10:09 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 17, 2012 at 1:07 PM, Antoine Labour <piman@chromium.org>
> wrote:
> >
> >
> > On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> >>
> >> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
> >> > wrote:
> >> >>
> >> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour <piman@chromium.org
> >
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
> >> >> >>
> >> >> >> Sad news. This got reverted for breaking the build.
> >> >> >>
> >> >> >> Apparently you can have DCHECKS enabled in an official build. So
> >> >> >> guarding
> >> >> >> on
> >> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
> >> >> >
> >> >> >
> >> >> > It looks like DCHECK, by design, will still reference the variables
> >> >> > in
> >> >> > the
> >> >> > condition when compiled out.
> >> >> > Still, doesn't mean we need to fork the style guide.
> >> >> >
> >> >> > Most of the places a DCHECK will be fine. In a few places where we
> >> >> > want
> >> >> > to
> >> >> > avoid the cost of keeping track of some variables, we can #ifdef it
> >> >> > on
> >> >> > some
> >> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
> >> >> > condition.
> >> >> > But limit it to those places.
> >> >> >
> >> >> > It doesn't have to affect the 95% of other DCHECKs.
> >> >>
> >> >> The problem that I see with this is that then some DCHECKs are run in
> >> >> different build conditions than others. It makes it trickier to know
> >> >> what's being tested. If you get a crash dump with a DCHECK failing,
> >> >> and there are guarded DCHECKs above it, did they pass or were they
> >> >> compiled out?
> >> >
> >> >
> >> > Then make it a CHECK?
> >>
> >> I'm not sure how that helps. You still don't know if the condition in
> >> the CHECK passed or it was just not compiled in.
> >
> >
> > Just make the condition obvious? e.g. NDEBUG?
> > I think that's entirely orthogonal.
>
> NDEBUG doesn't work for try bots :( This is the whole problem..
>

Of course they do, but only on the debug bots.  They don't on _rel bots or
the rel-with-dchecks bots.  But this is a really really minor issue.  We
have debug bots on the waterfall and developers are (hopefully) running
tests in debug.  I don't think it's worth being any different from the rest
of the chrome world here.  Write CHECK()s when you want it to crash in the
field, DCHECK() when you want it only happening on debug or rel-with-dcheck
bots, and #ifndef NDEBUG if you want to guard some data and not have it
compiled in out in the field.  The last category won't run on all of the
bots we have but it'll run on enough to give us coverage.

I haven't personally seen any of the last category of checks fail on the
bots that I can remember - they're really the most useful for developers
who are hacking on stuff locally and want to make sure it's still working.
 Those developers are hopefully using real debug builds on their machine.

- James


> >>
> >> I think we should
> >> have all our checks enabled/disabled together.
> >
> >
> > The same argument could go with cc vs rest-of-chrome.
>
> Yes except they can't appear together in the same method, making one
> dependent on the other.
>
> > We're really only talking about a few asserts here, that are complex
> enough
> > that they need specific tracking that we don't want to do in official
> > builds. It's a fairly rare case. I don't think that justifies changing
> every
> > assert, subtly changing the semantic, causing additional cognitive cost
> for
> > readers of the code, and extra reviews cycles that will constantly happen
> > because of the style fork.
>
> Well, FWIW there will be no extra review cycles since we made a
> PRESUBMIT. This saves us from having extra review cycles because
> someone chose the wrong guard/didn't remember to guard/etc.
>
> > Antoine
> >
> >
> >>
> >>
> >> --
> >>
> >>
> >
> > --
> >
> >
>

[danakj, 2012-10-17 17:23:33]

On Wed, Oct 17, 2012 at 1:19 PM, James Robinson <jamesr@google.com> wrote:
>
>
> On Wed, Oct 17, 2012 at 10:09 AM, Dana Jansens <danakj@chromium.org> wrote:
>>
>> On Wed, Oct 17, 2012 at 1:07 PM, Antoine Labour <piman@chromium.org>
>> wrote:
>> >
>> >
>> > On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org>
>> > wrote:
>> >>
>> >> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour
>> >> >> <piman@chromium.org>
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
>> >> >> >>
>> >> >> >> Sad news. This got reverted for breaking the build.
>> >> >> >>
>> >> >> >> Apparently you can have DCHECKS enabled in an official build. So
>> >> >> >> guarding
>> >> >> >> on
>> >> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
>> >> >> >
>> >> >> >
>> >> >> > It looks like DCHECK, by design, will still reference the
>> >> >> > variables
>> >> >> > in
>> >> >> > the
>> >> >> > condition when compiled out.
>> >> >> > Still, doesn't mean we need to fork the style guide.
>> >> >> >
>> >> >> > Most of the places a DCHECK will be fine. In a few places where we
>> >> >> > want
>> >> >> > to
>> >> >> > avoid the cost of keeping track of some variables, we can #ifdef
>> >> >> > it
>> >> >> > on
>> >> >> > some
>> >> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
>> >> >> > condition.
>> >> >> > But limit it to those places.
>> >> >> >
>> >> >> > It doesn't have to affect the 95% of other DCHECKs.
>> >> >>
>> >> >> The problem that I see with this is that then some DCHECKs are run
>> >> >> in
>> >> >> different build conditions than others. It makes it trickier to know
>> >> >> what's being tested. If you get a crash dump with a DCHECK failing,
>> >> >> and there are guarded DCHECKs above it, did they pass or were they
>> >> >> compiled out?
>> >> >
>> >> >
>> >> > Then make it a CHECK?
>> >>
>> >> I'm not sure how that helps. You still don't know if the condition in
>> >> the CHECK passed or it was just not compiled in.
>> >
>> >
>> > Just make the condition obvious? e.g. NDEBUG?
>> > I think that's entirely orthogonal.
>>
>> NDEBUG doesn't work for try bots :( This is the whole problem..
>
>
> Of course they do, but only on the debug bots.  They don't on _rel bots or
> the rel-with-dchecks bots.  But this is a really really minor issue.

The CQ bots are release bots with dchecks on, those are the trybots
I'm worrying about. Missing checks in the CQ can mean more closing the
tree and causing reverts. Shouldn't we be trying our best to run all
our debug checks on the CQ bots? How many times has the EWS caught
debug checks failing in unit tests?

If the majority feel this way then fine, but it seems bad to me.

> We
> have debug bots on the waterfall and developers are (hopefully) running
> tests in debug.  I don't think it's worth being any different from the rest
> of the chrome world here.  Write CHECK()s when you want it to crash in the
> field, DCHECK() when you want it only happening on debug or rel-with-dcheck
> bots, and #ifndef NDEBUG if you want to guard some data and not have it
> compiled in out in the field.  The last category won't run on all of the
> bots we have but it'll run on enough to give us coverage.
>
> I haven't personally seen any of the last category of checks fail on the
> bots that I can remember - they're really the most useful for developers who
> are hacking on stuff locally and want to make sure it's still working.
> Those developers are hopefully using real debug builds on their machine.
>
> - James
>
>>
>> >>
>> >> I think we should
>> >> have all our checks enabled/disabled together.
>> >
>> >
>> > The same argument could go with cc vs rest-of-chrome.
>>
>> Yes except they can't appear together in the same method, making one
>> dependent on the other.
>>
>> > We're really only talking about a few asserts here, that are complex
>> > enough
>> > that they need specific tracking that we don't want to do in official
>> > builds. It's a fairly rare case. I don't think that justifies changing
>> > every
>> > assert, subtly changing the semantic, causing additional cognitive cost
>> > for
>> > readers of the code, and extra reviews cycles that will constantly
>> > happen
>> > because of the style fork.
>>
>> Well, FWIW there will be no extra review cycles since we made a
>> PRESUBMIT. This saves us from having extra review cycles because
>> someone chose the wrong guard/didn't remember to guard/etc.
>>
>> > Antoine
>> >
>> >
>> >>
>> >>
>> >> --
>> >>
>> >>
>> >
>> > --
>> >
>> >
>
>

[jamesr, 2012-10-17 17:28:02]

On Wed, Oct 17, 2012 at 10:23 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 17, 2012 at 1:19 PM, James Robinson <jamesr@google.com> wrote:
> >
> >
> > On Wed, Oct 17, 2012 at 10:09 AM, Dana Jansens <danakj@chromium.org>
> wrote:
> >>
> >> On Wed, Oct 17, 2012 at 1:07 PM, Antoine Labour <piman@chromium.org>
> >> wrote:
> >> >
> >> >
> >> > On Wed, Oct 17, 2012 at 9:47 AM, Dana Jansens <danakj@chromium.org>
> >> > wrote:
> >> >>
> >> >> On Wed, Oct 17, 2012 at 12:45 PM, Antoine Labour <piman@chromium.org
> >
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Wed, Oct 17, 2012 at 9:38 AM, Dana Jansens <danakj@chromium.org
> >
> >> >> > wrote:
> >> >> >>
> >> >> >> On Wed, Oct 17, 2012 at 12:34 PM, Antoine Labour
> >> >> >> <piman@chromium.org>
> >> >> >> wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> > On Wed, Oct 17, 2012 at 8:37 AM, <danakj@chromium.org> wrote:
> >> >> >> >>
> >> >> >> >> Sad news. This got reverted for breaking the build.
> >> >> >> >>
> >> >> >> >> Apparently you can have DCHECKS enabled in an official build.
> So
> >> >> >> >> guarding
> >> >> >> >> on
> >> >> >> >> LOGGING_IS_OFFICIAL_BUILD won't do. Going back to CC_DCHECK.
> >> >> >> >
> >> >> >> >
> >> >> >> > It looks like DCHECK, by design, will still reference the
> >> >> >> > variables
> >> >> >> > in
> >> >> >> > the
> >> >> >> > condition when compiled out.
> >> >> >> > Still, doesn't mean we need to fork the style guide.
> >> >> >> >
> >> >> >> > Most of the places a DCHECK will be fine. In a few places where
> we
> >> >> >> > want
> >> >> >> > to
> >> >> >> > avoid the cost of keeping track of some variables, we can #ifdef
> >> >> >> > it
> >> >> >> > on
> >> >> >> > some
> >> >> >> > condition, and #ifdef the corresponding DCHECK(s), on the same
> >> >> >> > condition.
> >> >> >> > But limit it to those places.
> >> >> >> >
> >> >> >> > It doesn't have to affect the 95% of other DCHECKs.
> >> >> >>
> >> >> >> The problem that I see with this is that then some DCHECKs are run
> >> >> >> in
> >> >> >> different build conditions than others. It makes it trickier to
> know
> >> >> >> what's being tested. If you get a crash dump with a DCHECK
> failing,
> >> >> >> and there are guarded DCHECKs above it, did they pass or were they
> >> >> >> compiled out?
> >> >> >
> >> >> >
> >> >> > Then make it a CHECK?
> >> >>
> >> >> I'm not sure how that helps. You still don't know if the condition in
> >> >> the CHECK passed or it was just not compiled in.
> >> >
> >> >
> >> > Just make the condition obvious? e.g. NDEBUG?
> >> > I think that's entirely orthogonal.
> >>
> >> NDEBUG doesn't work for try bots :( This is the whole problem..
> >
> >
> > Of course they do, but only on the debug bots.  They don't on _rel bots
> or
> > the rel-with-dchecks bots.  But this is a really really minor issue.
>
> The CQ bots are release bots with dchecks on, those are the trybots
> I'm worrying about. Missing checks in the CQ can mean more closing the
> tree and causing reverts. Shouldn't we be trying our best to run all
> our debug checks on the CQ bots? How many times has the EWS caught
> debug checks failing in unit tests?
>

I'm pretty sure WebKit EWS bots are release, so zero times.


>
> If the majority feel this way then fine, but it seems bad to me.
>

Let's see if it's a problem - I doubt it will be.

- James


> > We
> > have debug bots on the waterfall and developers are (hopefully) running
> > tests in debug.  I don't think it's worth being any different from the
> rest
> > of the chrome world here.  Write CHECK()s when you want it to crash in
> the
> > field, DCHECK() when you want it only happening on debug or
> rel-with-dcheck
> > bots, and #ifndef NDEBUG if you want to guard some data and not have it
> > compiled in out in the field.  The last category won't run on all of the
> > bots we have but it'll run on enough to give us coverage.
> >
> > I haven't personally seen any of the last category of checks fail on the
> > bots that I can remember - they're really the most useful for developers
> who
> > are hacking on stuff locally and want to make sure it's still working.
> > Those developers are hopefully using real debug builds on their machine.
> >
> > - James
> >
> >>
> >> >>
> >> >> I think we should
> >> >> have all our checks enabled/disabled together.
> >> >
> >> >
> >> > The same argument could go with cc vs rest-of-chrome.
> >>
> >> Yes except they can't appear together in the same method, making one
> >> dependent on the other.
> >>
> >> > We're really only talking about a few asserts here, that are complex
> >> > enough
> >> > that they need specific tracking that we don't want to do in official
> >> > builds. It's a fairly rare case. I don't think that justifies changing
> >> > every
> >> > assert, subtly changing the semantic, causing additional cognitive
> cost
> >> > for
> >> > readers of the code, and extra reviews cycles that will constantly
> >> > happen
> >> > because of the style fork.
> >>
> >> Well, FWIW there will be no extra review cycles since we made a
> >> PRESUBMIT. This saves us from having extra review cycles because
> >> someone chose the wrong guard/didn't remember to guard/etc.
> >>
> >> > Antoine
> >> >
> >> >
> >> >>
> >> >>
> >> >> --
> >> >>
> >> >>
> >> >
> >> > --
> >> >
> >> >
> >
> >
>