1117259 - Disable the unused-local-typedef warning if clang supports it

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

This warning is typically triggered by code which implements
some kind of assertion macro, and it's probably not a good
indicator of bugs anyway, so there is no good reason to keep
it on.

Comment 1

[(no longer active)]

Attachment: Disable the no-unused-local-typedef warning if clang supports it

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8543461 [details] [diff] [review]
Disable the no-unused-local-typedef warning if clang supports it

># HG changeset patch
># User Ehsan Akhgari <[e-mail]>
>
>Bug 1117259 - Disable the no-unused-local-typedef warning if clang supports it; r=gps
>
>This warning is typically triggered by code which implements
>some kind of assertion macro, and it's probably not a good
>indicator of bugs anyway, so there is no good reason to keep
>it on.

I think gcc enabled this warning a couple releases ago and the result of the discussion then was to fix the warnings since there was some case in which it found bugs and asserting this way is silly.  Since I think we fixed all the cases gcc warns about I'm curious where clang warns.

ccing dholbert since I think he knows the history better

>     # -Wno-inline-new-delete - we inline 'new' and 'delete' in mozalloc
>+    # -Wno-unused-local-typedef - catches unused typedefs, which are commonly used in assertion macros
>     #   for performance reasons, and because GCC and clang accept it (though
>     #   clang warns about it).

you butchered that comment ;)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> I think gcc enabled this warning a couple releases ago and the result of the
> discussion then was to fix the warnings since there was some case in which
> it found bugs and asserting this way is silly.

That matches my recollection. (This happened in bug 851237.)  I posted a strawman patch there to disable this warning, but we didn't end up needing it because Waldo fixed all of the instances that caused fatal warnings. (and we've fixed more as we've discovered them since then.)

> Since I think we fixed all
> the cases gcc warns about I'm curious where clang warns.

I'm not 100% sure we fixed *all* of the places where GCC warns, but we fixed all the ones in FAIL_ON_WARNINGS directories at least (or else we wouldn't be able to build).

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #0)
> This warning is typically triggered by code which implements
> some kind of assertion macro, and it's probably not a good
> indicator of bugs anyway, so there is no good reason to keep
> it on.

Bug 851237 comment 3 disagrees; GCC enabled it by default specifically because it *did* catch a bug.

Where are you hitting this warning & wanting to silence it?  IIRC in the past we've mostly hit this for typedefs-that-really-want-to-be-static-asserts. If that's what you're seeing, we presumably want to use a static_assert or (if it's in 3rd-party code) maybe disable this warning *just for that code*.

Comment 4

[Jeff Walden [:Waldo]]

Static assertion macros aside (and those are almost completely gone now), I still don't see a reason why unused local typedefs are valuable enough to permit as dead code.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8543461 [details] [diff] [review]
Disable the no-unused-local-typedef warning if clang supports it

Review of attachment 8543461 [details] [diff] [review]:
-----------------------------------------------------------------

What an annoying DRY violation. Did I see a bug filed from you to fix this? All my bugmail from today is a bit fuzzy.

Anyway, I'll r+ from build land. But, I insist on sign-off from another C++ whiz that this is wanted, as comments imply this may not be wanted.

Comment 6

[(no longer active)]

Attachment: example build failures in WebRTC

I don't remember if this happens in other parts of the tree as well.

Randell, can we fix these in our tree?  Or do the fixes need to go upstream?

Comment 7

[(no longer active)]

And this grossness is probably why gcc doesn't warn about these: <http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h#113>

Comment 8

[Daniel Holbert [:dholbert]]

Yup, exactly -- I was just typing up a comment to explain that. :) We suppressed this issue for GCC via that #pragma, in bug 1020661.  See mozilla-central changeset 177b9f5de1b6.  IIRC, we did that instead of directly fixing the assertion macros because that change is easier to reapply when we pull in updates from upstream.

That patch specifically excluded clang, since (per bug 1020661 comment 8) clang versions at that time didn't recognize the warning, and spammed their own warning about it.

We could conceivably expand that #pragma to suppress this warning in clang versions that know about it -- maybe with a version-check?  Alternately, we could fix the static assertion macros, but that may cause more painful bitrot when we take WebRTC updates.

Comment 9

[Daniel Holbert [:dholbert]]

Also, as noted at the end of bug 1020661 comment 0, there exist more-updated versions of the scoped_ptr.h header that don't cause this problem. If & when WebRTC grabs an updated version of that header (and we get the updated WebRTC code), this will no longer be an issue.

Comment 10

[(no longer active)]

Well, let's wait to see what Randell says.  But yes, this warning is new to clang.

Comment 11

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Yup, exactly -- I was just typing up a comment to explain that. :) We
> suppressed this issue for GCC via that #pragma, in bug 1020661.  See
> mozilla-central changeset 177b9f5de1b6.  IIRC, we did that instead of
> directly fixing the assertion macros because that change is easier to
> reapply when we pull in updates from upstream.

Also, that's a terrible way of disabling warnings in general.  ;-)  Something like <http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/basictypes.h#164> would have been much better, and it works in both clang and gcc.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Also, that's a terrible way of disabling warnings in general.  ;-) 
> Something like [ COMPILE_ASSERT_UNUSED_ATTRIBUTE ] would have been much better

(I completely agree.  IIRC, the reason I went with the #pragma in that particular case was to avoid changing any lines of code, to minimize bitrot-pain when get our next WebRTC upgrade from trunk.  Per bug 1020661 comment 4, our reapply-local-patches process for WebRTC is not easy.)

Comment 13

[Daniel Holbert [:dholbert]]

(As you suggest, though, I suspect we could address this with a COMPILE_ASSERT_UNUSED_ATTRIBUTE dance here:
https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/system_wrappers/interface/compile_assert.h
If that works, that's probably the best way of working around this minimally & locally, at this point.)

Comment 14

[(no longer active)]

This <https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/system_wrappers/interface/compile_assert.h#33> will probably make that not work reliably.

Comment 15

[Daniel Holbert [:dholbert]]

It only wouldn't work if some .cpp file ends up (directly or indirectly) #including a different COMPILE_ASSERT-providing header before this one, right?

If that ends up happening (& if it's even possible -- I don't know how many headers we have that provide a COMPILE_ASSERT macro), I'd think we can & should just apply the same dance to the other COMPILE_ASSERT definition.  For the one you linked (in ipc/chromium/src/base/basictypes.h), we already have the UNUSED_ATTRIBUTE hack, so we should be fine.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Also, as noted at the end of bug 1020661 comment 0, there exist more-updated
> versions of the scoped_ptr.h header that don't cause this problem. If & when
> WebRTC grabs an updated version of that header (and we get the updated
> WebRTC code), this will no longer be an issue.

See Bug 1109428; I have a webrtc.org update queued up and green on mac, linux and windows.  Still working on b2g and android.

And yes, stuff like this should preferably land up upstream.  Carrying excess local patches just makes the upgrades more painful due to conflicts.

Comment 17

[(no longer active)]

Can you help fix this upstream, please?

Comment 18

[(no longer active)]

Randell: ping?

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> Can you help fix this upstream, please?

(FWIW: per comment 9, I think the best way to fix this upstream is for WebRTC to grab a newer version of Google's "scoped_ptr.h" header -- specifically, a version that doesn't use a typedef for "type_must_be_complete" in ~scoped_ptr.

For example, WebRTC could use the version that breakpad switched to in https://code.google.com/p/google-breakpad/issues/detail?id=534 . Though there's probably even a newer version now.)

Comment 20

[(no longer active)]

I need this for bug 1201938 since gstreamer code is affected by the same issue now, so I'll just check in this patch!

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fe4dadb893

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a7fe4dadb893

Comment 23

[Nicholas Nethercote [inactive]]

In GCC, according to https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html, this option is called -Wunused-local-typedefs -- note the extra 's' at the end!

So I think this patch actually had no effect. Or does the option name not have the trailing 's' for clang?

Comment 24

[Nicholas Nethercote [inactive]]

> Or does the option name not have the trailing 's' for clang?

Looks like that's the case: https://github.com/llvm-mirror/clang/search?utf8=%E2%9C%93&q=unused-local-typedef

Ugh.

Comment 25

[Nicholas Nethercote [inactive]]

Although, the landed patch used MOZ_CXX_SUPPORTS_WARNING instead of MOZ_C_SUPPORTS_WARNING for the C warnings, so that part is bogus. So looks this isn't actually enabled for C code. I'll fix this as part of a clean-up I'm doing of this code (bug to come).

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #17)
> > Can you help fix this upstream, please?
> 
> (FWIW: per comment 9, I think the best way to fix this upstream is for
> WebRTC to grab a newer version of Google's "scoped_ptr.h" header --
> specifically, a version that doesn't use a typedef for
> "type_must_be_complete" in ~scoped_ptr.
> 
> For example, WebRTC could use the version that breakpad switched to in
> https://code.google.com/p/google-breakpad/issues/detail?id=534 . Though
> there's probably even a newer version now.)

We'll be updating from webrtc.org stable upstream branch 48 or maybe 49 in the next month or two.  We can check if that has the changes you want (just checking if webrtc.org trunk has the changes you want would be a good indication that 48 has what you want or not).  Note: I'm on PTO, but you can check on webrtc.org pretty easily.