1019050 - GCC 4.8 unused-local-typedefs warning/error in dom/media after WebRTC 3.50 update

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

> 0:16.10 In file included from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/aligned_malloc.h:22:0,
> 0:16.10                  from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/common_video/plane.h:14,
> 0:16.10                  from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/common_video/interface/i420_video_frame.h:18,
> 0:16.10                  from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h:22,
> 0:16.10                  from ../../dist/include/MediaEngineWebRTC.h:51,
> 0:16.10                  from /home/nchen/gecko-dev/dom/media/MediaManager.cpp:49,
> 0:16.10                  from /home/nchen/gecko-dev/objdir-android/dom/media/Unified_cpp_dom_media0.cpp:15:
> 0:16.10 /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h: In constructor 'webrtc::DefaultDeleter<T>::DefaultDeleter(const webrtc::DefaultDeleter<U>&)':
> 0:16.10 /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h:136:76: error: typedef 'U_ptr_must_implicitly_convert_to_T_ptr' locally defined but not used [-Werror=unused-local-typedefs]
> 0:16.10      COMPILE_ASSERT((webrtc::is_convertible<U*, T*>::value),

One of many warnings being treated as errors in dom/media.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Fix unused-local-typedefs warning/error after WebRTC update (v1)

Simplest fix to disable the warning. This patch applies to both GCC and clang.

Comment 2

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

Comment on attachment 8432606 [details] [diff] [review]
Fix unused-local-typedefs warning/error after WebRTC update (v1)

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

r+, though I'd welcome a patch against upstream trunk (not 3.50) to resolve any issues like this so we can upstream it.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=8b3ad8f2d301

Comment 4

[Daniel Holbert [:dholbert]]

We should really fix the build warnings. I've filed bug 1020643 and bug 1020661 on that. (Have a patch for the former; not yet sure what the right fix is for the latter, since it's in upstream webrtc code.)

Comment 5

[Daniel Holbert [:dholbert]]

(Ideally we should add a comment alongside the warning-disabling-chunk, saying that it's a hackaround for bug 1020661 & we can drop that chunk once that bug is fixed.)

Comment 6

[Daniel Holbert [:dholbert]]

(I'm not objecting to landing the patch as-is, though I'd suggest at least pushing a DONTBUILD followup to add a comment along the lines of Comment 5.)

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Fix unused-local-typedefs warning/error after WebRTC update (v1.1)

Added a comment.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

We should back this out after bug 1020661 is fixed. Not sure what the right dependency is in this case, but at least it'll generate a bugmail once bug 1020661 is fixed :)

Comment 9

[Daniel Holbert [:dholbert]]

Actually, a few thoughts on this patch:
 (1) Sheriffs are now requiring Try pushes before they'll honor 'checkin-needed', per a recent newsgroup thread. Hence, removing keyword for the moment (a sheriff would've done so when they got around to triaging checkin-needed anyway).

 (2) I suspect this will cause bustage in old versions of GCC that don't support this warning -- GCC errors out if you pass it command-line args about warnings that it doesn't recognize. (This is why we have the MOZ_CXX_SUPPORTS_WARNING() macro in configure.in) However, this may not be an issue since we already require GCC versions recent enough to have C++11 features like static_cast and 'auto', which may be more recent than when this warning was added to GCC. So, tl;dr, if this passes a Try run that includes B2G, probably no need to worry about this.

 (3) It may be a little better to use -Wno-error=unused-local-typedefs (note the "Wno-error=", instead of "Wno-"), which should make us still display the warnings but not treat them as errors. (This will let people still see other useful instances of this warning in dom/media and have a chance to address them, and prevent us (hopefully) from accidentally introducing additional instances.)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

There's a try push from comment 5 that looks okay, https://tbpl.mozilla.org/?tree=Try&rev=8b3ad8f2d301

I do remember -Wno- being better supported/tolerated than -Wno-error= though (for clang I think).

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Oops comment 3, not comment 5

Comment 12

[Daniel Holbert [:dholbert]]

Ah, apologies - I totally overlooked that Try push. (I should trust my eyes less, and lean on find-in-page more. :))

Given that the try push (particularly the B2G section w/ old GCC versions) was all green, I guess the "this may not be an issue" in comment 9 part (2) is probably the case. Hooray! And I don't really care about -Wno- vs -Wno-error (part (3)) for this particular warning.

So, feel free to add back checkin-needed -- sorry for mistakenly clearing it.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

No worries :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb57bc2a213

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fb57bc2a213