Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(58)

Issue 1000493003: Oilpan: fix gcc compilation. (Closed)

Created:
5 years, 9 months ago by sof
Modified:
5 years, 9 months ago
CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium)
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Switch to a static_assert() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M Source/platform/heap/ThreadState.h View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18 (3 generated)
sof
Please take a look. I'm fine with alternatively dropping the ASSERT(), fwiw. But a change ...
5 years, 9 months ago (2015-03-11 09:04:15 UTC) #2
tkent
Do we need to use *trailing* return type? I have never seen usage of trailing ...
5 years, 9 months ago (2015-03-12 01:06:27 UTC) #3
sof
On 2015/03/12 01:06:27, tkent wrote: > Do we need to use *trailing* return type? I ...
5 years, 9 months ago (2015-03-12 06:09:24 UTC) #4
tkent
On 2015/03/12 06:09:24, sof wrote: > On 2015/03/12 01:06:27, tkent wrote: > > Do we ...
5 years, 9 months ago (2015-03-12 23:31:24 UTC) #5
Nico
Can this just be a static_assert instead of an ASSERT?
5 years, 9 months ago (2015-03-16 14:18:29 UTC) #7
sof
On 2015/03/16 14:18:29, Nico (traveling) wrote: > Can this just be a static_assert instead of ...
5 years, 9 months ago (2015-03-16 14:21:01 UTC) #8
Nico
static_assert(sizeof(T::invokePreFinalizer) > 0) or static_assert(static_cast<decltype(T::invokePreFinalizer)*>(nullptr) == nullptr) or something like that (not tested). (My point ...
5 years, 9 months ago (2015-03-16 14:31:50 UTC) #9
sof
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) ...
5 years, 9 months ago (2015-03-16 14:33:42 UTC) #10
sof
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) ...
5 years, 9 months ago (2015-03-16 18:45:41 UTC) #11
sof
Sorry for letting this one hang around a while..now switched over to a suitably wrapped ...
5 years, 9 months ago (2015-03-19 15:19:51 UTC) #12
Nico
On 2015/03/19 15:19:51, sof wrote: > Sorry for letting this one hang around a while..now ...
5 years, 9 months ago (2015-03-19 16:09:53 UTC) #13
tkent
lgtm
5 years, 9 months ago (2015-03-19 22:13:00 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1000493003/20001
5 years, 9 months ago (2015-03-19 22:13:22 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192201
5 years, 9 months ago (2015-03-19 22:16:53 UTC) #17
haraken
5 years, 9 months ago (2015-03-19 23:13:32 UTC) #18
Message was sent while issue was closed.
LGTM

Powered by Google App Engine
This is Rietveld 408576698