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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

> 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.
Simplest fix to disable the warning. This patch applies to both GCC and clang.
Attachment #8432606 - Flags: review?(rjesup)
Attachment #8432606 - Flags: review?(gps)
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.
Attachment #8432606 - Flags: review?(rjesup) → review+
Attachment #8432606 - Flags: review?(gps) → review+
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.)
(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.)
(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.)
Version: unspecified → Trunk
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 :)
Depends on: 1020661
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.)
Flags: needinfo?(nchen)
Keywords: checkin-needed
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).
Flags: needinfo?(nchen)
Keywords: checkin-needed
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.
No worries :)
https://hg.mozilla.org/mozilla-central/rev/5fb57bc2a213
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.