base: Change DCHECK_IS_ON to a macro DCHECK_IS_ON().

base: Change DCHECK_IS_ON to a macro DCHECK_IS_ON().

This ensures that if the header is not included, and a DCHECK is guarded
by this check, that the file will fail to compile instead of silently
compiling the DCHECK out.

For example:

#if DCHECK_IS_ON
DCHECK(SomeThing());
#endif

This example would be compiled out if DCHECK_IS_ON was not defined due
to not including the logging.h header.

Instead, this will fail to compile:

#if DCHECK_IS_ON()
DCHECK(SomeThing());
#endif

R=thakis@chromium.org

Committed: https://crrev.com/e649f573a38b00bb20fe0925098251a4ff184566
Cr-Commit-Position: refs/heads/master@{#310626}

[danakj, 2015-01-06 23:40:10]

danakj@chromium.org changed reviewers:
+ thakis@chromium.org

[danakj, 2015-01-06 23:41:17]

I was kinda on the fence about the .js changes but I figured consistency >
churn?

[danakj, 2015-01-06 23:42:47]

Oops, upload ate my # lines from the description, fixed.

[Nico, 2015-01-06 23:46:16]

https://codereview.chromium.org/842523002/diff/1/android_webview/browser/hard...
File android_webview/browser/hardware_renderer.cc (left):

https://codereview.chromium.org/842523002/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:120: #if DCHECK_IS_ON
Can we make DCHECK_IS_ON a function-style macro? `#if DCHECK_IS_ON()` will fail
instead of silently be 0, and we don't need double negation that way.

[danakj, 2015-01-06 23:55:10]

https://codereview.chromium.org/842523002/diff/1/android_webview/browser/hard...
File android_webview/browser/hardware_renderer.cc (left):

https://codereview.chromium.org/842523002/diff/1/android_webview/browser/hard...
android_webview/browser/hardware_renderer.cc:120: #if DCHECK_IS_ON
On 2015/01/06 23:46:15, Nico wrote:
> Can we make DCHECK_IS_ON a function-style macro? `#if DCHECK_IS_ON()` will
fail
> instead of silently be 0, and we don't need double negation that way.

Oh that's a good point. I thought no, because we used to have that and it was a
problem. But that's because it resolved to code that had to execute at runtime,
not just to a constant that the preprocessor can use.

[danakj, 2015-01-07 00:03:12]

PTAL. Updated description to match.

[Nico, 2015-01-07 00:12:13]

lgtm

Great catch, btw :-)

https://codereview.chromium.org/842523002/diff/40001/chromecast/browser/andro...
File chromecast/browser/android/external_video_surface_container_impl.cc
(right):

https://codereview.chromium.org/842523002/diff/40001/chromecast/browser/andro...
chromecast/browser/android/external_video_surface_container_impl.cc:46: JNIEnv*
env = base::android::AttachCurrentThread();
looks somewhat unrelated, but ok :-)

[danakj, 2015-01-07 04:40:17]

Cool thanks!

https://codereview.chromium.org/842523002/diff/40001/chromecast/browser/andro...
File chromecast/browser/android/external_video_surface_container_impl.cc
(right):

https://codereview.chromium.org/842523002/diff/40001/chromecast/browser/andro...
chromecast/browser/android/external_video_surface_container_impl.cc:46: JNIEnv*
env = base::android::AttachCurrentThread();
On 2015/01/07 00:12:13, Nico wrote:
> looks somewhat unrelated, but ok :-)

Er. I dunno how that is there. I'll remove it before landing. Musta picked it up
during rebase somehow.

[danakj, 2015-01-07 17:49:45]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[danakj, 2015-01-07 17:49:46]

+sievers for OWNERS of what you can

[danakj, 2015-01-07 17:50:28]

danakj@chromium.org changed reviewers:
+ jamesr@chromium.org

[danakj, 2015-01-07 17:50:29]

+jamesr for mojo/

[danakj, 2015-01-07 17:59:24]

danakj@chromium.org changed reviewers:
+ benm@chromium.org

[danakj, 2015-01-07 17:59:25]

+benm for android_webview/

[danakj, 2015-01-07 18:00:20]

danakj@chromium.org changed reviewers:
+ jyasskin@chromium.org

[danakj, 2015-01-07 18:00:21]

+jyasskin for extensions/

[benm (inactive), 2015-01-07 18:01:09]

On 2015/01/07 17:59:25, danakj wrote:
> +benm for android_webview/

lgtm

[danakj, 2015-01-07 18:04:19]

danakj@chromium.org changed reviewers:
+ erikwright@chromium.org, maniscalco@chromium.org

[danakj, 2015-01-07 18:04:20]

+maniscalco for sync/

+erikwright for components/

[danakj, 2015-01-07 18:09:22]

danakj@chromium.org changed reviewers:
+ damienv@chromium.org, rvargas@chromium.org

[danakj, 2015-01-07 18:09:23]

+damienv for chromecast/

+rvargas for net/

[maniscalco, 2015-01-07 18:13:03]

sync/ lgtm

[lcwu1, 2015-01-07 18:49:55]

lcwu@chromium.org changed reviewers:
+ lcwu@chromium.org

[lcwu1, 2015-01-07 18:49:57]

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
File chromecast/base/metrics/cast_histograms.h (right):

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
chromecast/base/metrics/cast_histograms.h:16: constant_histogram_name,
histogram_add_method_invocation,                  \
Any reason why you want to re-format the code here (i.e. moving the continuation
character to the end of the line)? The existing format seems to be consistent to
a couple of examples in Chromium (e.g. base/metrics/histogram.h) that I quickly
looked at.

[Nico, 2015-01-07 19:02:50]

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
File chromecast/base/metrics/cast_histograms.h (right):

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
chromecast/base/metrics/cast_histograms.h:16: constant_histogram_name,
histogram_add_method_invocation,                  \
On 2015/01/07 18:49:56, lcwu1 wrote:
> Any reason why you want to re-format the code here (i.e. moving the
continuation
> character to the end of the line)? The existing format seems to be consistent
to
> a couple of examples in Chromium (e.g. base/metrics/histogram.h) that I
quickly
> looked at.

The old formatting makes sense if you have to format things manually, since it's
a bunch of work to realign all the trailing \s. Now we have clang-format which
does this automatically, so that argument no longer holds. (Also, unless
clang-format does something that's hard to read, arguing about its formatting
decisions is very likely not a good use of time.)

[no sievers, 2015-01-07 19:09:09]

lgtm

[danakj, 2015-01-07 19:31:53]

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
File chromecast/base/metrics/cast_histograms.h (right):

https://codereview.chromium.org/842523002/diff/60001/chromecast/base/metrics/...
chromecast/base/metrics/cast_histograms.h:16: constant_histogram_name,
histogram_add_method_invocation,                  \
On 2015/01/07 19:02:50, Nico wrote:
> On 2015/01/07 18:49:56, lcwu1 wrote:
> > Any reason why you want to re-format the code here (i.e. moving the
> continuation
> > character to the end of the line)? The existing format seems to be
consistent
> to
> > a couple of examples in Chromium (e.g. base/metrics/histogram.h) that I
> quickly
> > looked at.
> 
> The old formatting makes sense if you have to format things manually, since
it's
> a bunch of work to realign all the trailing \s. Now we have clang-format which
> does this automatically, so that argument no longer holds. (Also, unless
> clang-format does something that's hard to read, arguing about its formatting
> decisions is very likely not a good use of time.)

Right, clang-format did this.

[lcwu1, 2015-01-07 19:46:31]

lgtm on chromecast/.

[jamesr, 2015-01-07 20:06:17]

jamesr@chromium.org changed reviewers:
+ rockot@chromium.org

[jamesr, 2015-01-07 20:06:18]

mojo lgtm although this code is mirrored from the mojo repo so we'll have to be
a bit careful to roll this correctly. Go ahead and land and I'll update the mojo
repo to reflect the change.

rockot@ - since this is an API change in base that edk depends on the order here
will have to be land in chromium then roll into mojo and update then roll back.
In between the first and second step rolling mojo into chrome will not work.
I'll take care of the chromium->mojo update and let you know when it's safe
again.

[Ken Rockot(use gerrit already), 2015-01-07 20:11:23]

sgtm

On Wed, Jan 7, 2015 at 12:06 PM, <jamesr@chromium.org> wrote:

> mojo lgtm although this code is mirrored from the mojo repo so we'll have
> to be
> a bit careful to roll this correctly. Go ahead and land and I'll update
> the mojo
> repo to reflect the change.
>
> rockot@ - since this is an API change in base that edk depends on the
> order here
> will have to be land in chromium then roll into mojo and update then roll
> back.
> In between the first and second step rolling mojo into chrome will not
> work.
> I'll take care of the chromium->mojo update and let you know when it's safe
> again.
>
> https://codereview.chromium.org/842523002/
>

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

[rvargas (doing something else), 2015-01-07 20:54:28]

net lgtm

[danakj, 2015-01-08 18:55:12]

rockot@ would you mind reviewing the extensions/ changes?

[danakj, 2015-01-08 18:56:06]

danakj@chromium.org changed reviewers:
+ sky@chromium.org

[danakj, 2015-01-08 18:56:08]

+sky for components/history/

[Ken Rockot(use gerrit already), 2015-01-08 19:02:28]

extensions lgtm

[sky, 2015-01-08 23:10:47]

Tricky, LGTM

[danakj, 2015-01-08 23:11:23]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:12:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/842523002/60001

[commit-bot: I haz the power, 2015-01-08 23:36:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-08 23:37:54]

Patchset 4 (id:??) landed as
https://crrev.com/e649f573a38b00bb20fe0925098251a4ff184566
Cr-Commit-Position: refs/heads/master@{#310626}

[dcheng, 2015-08-12 23:48:41]

FWIW, if you forget the ()s (or forget to #include the header
directly/indirectly), then

#if DCHECK_IS_ON

always evaluates to false. Maybe we should have a presubmit to enforce that "()"
are always present? nick@ noticed this by chance when he was experimenting with
a pre-2015 patch.

[danakj, 2015-08-12 23:55:12]

On Wed, Aug 12, 2015 at 4:49 PM, Dana Jansens <danakj@chromium.org> wrote:

> Really..? I thought it doesn't compile.. *checks*
>

So it does. Darn. Presubmit would be nice then.


>
> On Wed, Aug 12, 2015 at 4:48 PM, <dcheng@chromium.org> wrote:
>
>> FWIW, if you forget the ()s (or forget to #include the header
>> directly/indirectly), then
>>
>> #if DCHECK_IS_ON
>>
>> always evaluates to false. Maybe we should have a presubmit to enforce
>> that "()"
>> are always present? nick@ noticed this by chance when he was
>> experimenting with
>> a pre-2015 patch.
>>
>> https://codereview.chromium.org/842523002/
>>
>
>

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

[danakj, 2015-08-13 00:12:03]

Really..? I thought it doesn't compile.. *checks*

On Wed, Aug 12, 2015 at 4:48 PM, <dcheng@chromium.org> wrote:

> FWIW, if you forget the ()s (or forget to #include the header
> directly/indirectly), then
>
> #if DCHECK_IS_ON
>
> always evaluates to false. Maybe we should have a presubmit to enforce
> that "()"
> are always present? nick@ noticed this by chance when he was
> experimenting with
> a pre-2015 patch.
>
> https://codereview.chromium.org/842523002/
>

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