Oilpan: Replace checkHeader() with ASSERT(checkHeader())

Oilpan: Replace checkHeader() with ASSERT(checkHeader())

It seems compilers are not always smart enough to ignore checkHeader().
This CL replaces checkHeader() with ASSERT(checkHeader()) to make
it more explicit that it is nop in production code.

BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198039

[haraken, 2015-06-30 05:52:24]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org

[haraken, 2015-06-30 05:52:24]

PTAL

[keishi, 2015-06-30 05:56:11]

lgtm

[haraken, 2015-06-30 05:58:40]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-06-30 05:58:45]

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

[Yuta Kitamura, 2015-06-30 06:18:56]

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

[Yuta Kitamura, 2015-06-30 06:18:56]

Same here, not lgtm.

[haraken, 2015-06-30 06:22:23]

On 2015/06/30 06:18:56, Yuta Kitamura wrote:
> Same here, not lgtm.

This CL doesn't have your concern mentioned in
https://codereview.chromium.org/1208223003/#msg7, does it?

checkHeader() doesn't execute code that calls ASSERT inside it.

[Yuta Kitamura, 2015-06-30 06:26:59]

You're right, this seems fine. LGTM.

[Yuta Kitamura, 2015-06-30 06:30:57]

The reason why the function calls slow down the release
build is understandable in this case; that's because
the function cannot be inlined (the implementation is in
cpp file).

[commit-bot: I haz the power, 2015-06-30 07:33:12]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198039

[sof, 2015-06-30 08:02:33]

On 2015/06/30 06:30:57, Yuta Kitamura wrote:
> The reason why the function calls slow down the release
> build is understandable in this case; that's because
> the function cannot be inlined (the implementation is in
> cpp file).

Is that the case here?

A void checkHeader() has some merit in that it is easier to enable for release
builds, should that be needed for something.

[haraken, 2015-06-30 08:11:23]

On 2015/06/30 08:02:33, sof wrote:
> On 2015/06/30 06:30:57, Yuta Kitamura wrote:
> > The reason why the function calls slow down the release
> > build is understandable in this case; that's because
> > the function cannot be inlined (the implementation is in
> > cpp file).
> 
> Is that the case here?
> 
> A void checkHeader() has some merit in that it is easier to enable for release
> builds, should that be needed for something.

I'm not sure if it is the case here, but I guess putting assert-only functions
in ASSERT() would be a reasonable thing. If we want to enable it on release, can
we try release+assert?

[Yuta Kitamura, 2015-06-30 08:47:03]

On 2015/06/30 08:11:23, haraken wrote:
> I'm not sure if it is the case here, but I guess putting assert-only functions
> in ASSERT() would be a reasonable thing. If we want to enable it on release,
can
> we try release+assert?

We have a gyp variable "dcheck_always_on" for this exact purpose.
This should work for Blink's ASSERT()s, too.

[sof, 2015-06-30 08:49:49]

On 2015/06/30 08:47:03, Yuta Kitamura wrote:
> On 2015/06/30 08:11:23, haraken wrote:
> > I'm not sure if it is the case here, but I guess putting assert-only
functions
> > in ASSERT() would be a reasonable thing. If we want to enable it on release,
> can
> > we try release+assert?
> 
> We have a gyp variable "dcheck_always_on" for this exact purpose.
> This should work for Blink's ASSERT()s, too.

A big hammer. I don't really see the merit of this CL, but fine to make
checkHeader() a predicate.