Oilpan: fix gcc compilation.

Oilpan: fix gcc compilation.

To catch out erroneous uses of unregisterPreFinalizer() over
class types T that do not provide an implementation of invokePreFinalizer()
(by way of USING_PRE_FINALIZER(), most likely), unregisterPreFinalizer()
currently asserts for T::invokePreFinalizer being bound and non-null.

This unfortunately runs into gcc's -Waddress warning ("the address of
<function> will never be null"), as the static method is statically
known and bound (where correctly used.)

Rephrase the assert by wrapping up the check in a sizeof() so as to
steer clear of gcc's fussiness.

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

[sof, 2015-03-11 09:04:14]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, tkent@chromium.org

[sof, 2015-03-11 09:04:15]

Please take a look.

I'm fine with alternatively dropping the ASSERT(), fwiw. But a change is needed
to fix gcc building.

[tkent, 2015-03-12 01:06:27]

Do we need to use *trailing* return type?  I have never seen usage of trailing
return types in Chromium/Blink, and it's unclear if it is allowed or not
<https://chromium-cpp.appspot.com/>.

[sof, 2015-03-12 06:09:24]

On 2015/03/12 01:06:27, tkent wrote:
> Do we need to use *trailing* return type?  I have never seen usage of trailing
> return types in Chromium/Blink, and it's unclear if it is allowed or not
> <https://chromium-cpp.appspot.com/>.

It is mentioned & used as motivation in the thread attached to decltype().

Does it merit use here? It gets us out of a tight spot wrt -Waddress, but the
condition it checks for is arguably caught upon unregistration anyway.

[tkent, 2015-03-12 23:31:24]

On 2015/03/12 06:09:24, sof wrote:
> On 2015/03/12 01:06:27, tkent wrote:
> > Do we need to use *trailing* return type?  I have never seen usage of
trailing
> > return types in Chromium/Blink, and it's unclear if it is allowed or not
> > <https://chromium-cpp.appspot.com/>.
> 
> It is mentioned & used as motivation in the thread attached to decltype().

It was just an example for decltype.  I think we had no discussion on trailing
return type, and we need to make a chromium-dev thread for it.
I support just removing the ASSERT for now.

[Nico, 2015-03-16 14:18:28]

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

[Nico, 2015-03-16 14:18:29]

Can this just be a static_assert instead of an ASSERT?

[sof, 2015-03-16 14:21:01]

On 2015/03/16 14:18:29, Nico (traveling) wrote:
> Can this just be a static_assert instead of an ASSERT?

i think i tried that, but let me recheck..

[Nico, 2015-03-16 14:31:50]

static_assert(sizeof(T::invokePreFinalizer) > 0) or
static_assert(static_cast<decltype(T::invokePreFinalizer)*>(nullptr) == nullptr)
or something like that (not tested).

(My point here is that using a trailing return type for a function that returns
void, just to hide some checking there, makes the code really hard to read. I
think this CL is a good example why trailing return types shouldn't be allowed:
They make good people try to do clever things ;-) )

[sof, 2015-03-16 14:33:42]

On 2015/03/16 14:31:50, Nico (traveling) wrote:
> static_assert(sizeof(T::invokePreFinalizer) > 0) or
> static_assert(static_cast<decltype(T::invokePreFinalizer)*>(nullptr) ==
nullptr)
> or something like that (not tested).
> 
> (My point here is that using a trailing return type for a function that
returns
> void, just to hide some checking there, makes the code really hard to read. I
> think this CL is a good example why trailing return types shouldn't be
allowed:
> They make good people try to do clever things ;-) )

I don't think that is lost on anyone here, its use just gets out of a tight spot
but otherwise not too interesting a use.

[sof, 2015-03-16 18:45:41]

On 2015/03/16 14:31:50, Nico (traveling) wrote:
> static_assert(sizeof(T::invokePreFinalizer) > 0) or
> static_assert(static_cast<decltype(T::invokePreFinalizer)*>(nullptr) ==
nullptr)
> or something like that (not tested).
> 
> (My point here is that using a trailing return type for a function that
returns
> void, just to hide some checking there, makes the code really hard to read. I
> think this CL is a good example why trailing return types shouldn't be
allowed:
> They make good people try to do clever things ;-) )

static_assert(sizeof(&T::invokePreFinalizer) > 0, ...) works with gcc(-4.8.1)

It only appears triggerable for LifecycleObserver<>::invokePreFinalizer() right
now; https://codereview.chromium.org/1006253002/ drops that prefinalizer, and
the ones that remain (and their unregistrations) do not elicit the -Waddress
warning.

[sof, 2015-03-19 15:19:51]

Sorry for letting this one hang around a while..now switched over to a suitably
wrapped up static_assert().

Looks acceptable?

[Nico, 2015-03-19 16:09:53]

On 2015/03/19 15:19:51, sof wrote:
> Sorry for letting this one hang around a while..now switched over to a
suitably
> wrapped up static_assert().
> 
> Looks acceptable?

Lgtm, thanks!

[tkent, 2015-03-19 22:12:59]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-03-19 22:13:00]

lgtm

[commit-bot: I haz the power, 2015-03-19 22:13:22]

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

[commit-bot: I haz the power, 2015-03-19 22:16:53]

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

[haraken, 2015-03-19 23:13:32]

LGTM