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

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla43
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

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.
Assignee: nobody → ehsan
Comment on attachment 8543461 [details] [diff] [review]
Disable the no-unused-local-typedef warning if clang supports it

># HG changeset patch
># User Ehsan Akhgari <ehsan@mozilla.com>
>
>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 ;)
Flags: needinfo?(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*.
Flags: needinfo?(dholbert)
Summary: Disable the no-unused-local-typedef warning if clang supports it → Disable the unused-local-typedef warning if clang supports it
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 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.
Attachment #8543461 - Flags: review?(gps) → review+
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?
Flags: needinfo?(rjesup)
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.
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.
Well, let's wait to see what Randell says.  But yes, this warning is new to clang.
(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.
(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.)
(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.)
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.
(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.
Flags: needinfo?(rjesup)
Can you help fix this upstream, please?
Flags: needinfo?(rjesup)
Randell: ping?
(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.)
I need this for bug 1201938 since gstreamer code is affected by the same issue now, so I'll just check in this patch!
Blocks: 1201938
https://hg.mozilla.org/mozilla-central/rev/a7fe4dadb893
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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?
> 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.
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).
(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.
Flags: needinfo?(rjesup)
Duplicate of this bug: 982334
Depends on: 1243331

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.