clang 3.6 build warning (fatal in werror builds): MediaPromise.h:244:16: error: 'AssertOnDispatchThread' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

RESOLVED FIXED in Firefox 38

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
New build warning, using clang 3.6:
{
$SRC/ In file included from $SRC/dom/media/mediasource/MediaSource.cpp:7:
 In file included from $SRC/dom/media/mediasource/MediaSource.h:10:
 In file included from $SRC/dom/media/mediasource/MediaSourceDecoder.h:13:
 In file included from ../../../dist/include/MediaDecoder.h:200:
 In file included from ../../../dist/include/mozilla/CDMProxy.h:16:
 In file included from ../../../dist/include/mozilla/CDMCaps.h:16:
 In file included from ../../../dist/include/SamplesWaitingForKey.h:11:
 In file included from ../../../dist/include/MediaTaskQueue.h:15:
 ../../../dist/include/MediaPromise.h:244:16: error: 'AssertOnDispatchThread' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
   virtual void AssertOnDispatchThread()
}

This should be as simple as just adding MOZ_OVERRIDE, except there's a comment in the code saying that apparently our b2g desktop MacOS builds have trouble with that:
> 243   // Can't mark MOZ_OVERRIDE due to bug in clang builders we use for osx b2g desktop. :-(
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaPromise.h?rev=33588deed131#243

IIRC, bholley said that old clang versions have trouble with MOZ_OVERRIDE here due to a (clang) bug with inheriting from a templated class.

Unless we're about to drop support for that old clang version, we should probably use a "#define OVERRIDE_EXCEPT_IN_OLD_CLANG" here, to let people using new clang (like me) continue to build.
(Assignee)

Updated

3 years ago
Blocks: 1117034
(Assignee)

Updated

3 years ago
Assignee: nobody → dholbert
Hardware: x86_64 → All
(Assignee)

Comment 1

3 years ago
Created attachment 8558009 [details] [diff] [review]
fix v1

Hopefully this should do it.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e21d07297b9c

For comparison, here's a try run with just MOZ_OVERRIDE (which I'm expecting to fail on some platforms w/ old clang, based on the code-comment):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77cada79a209
Comment on attachment 8558009 [details] [diff] [review]
fix v1

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

::: dom/media/MediaPromise.h
@@ +21,5 @@
>  #ifdef _MSC_VER
>  #define __func__ __FUNCTION__
>  #endif
>  
> +#ifdef DEBUG

It is better to avoid wrapping this in #ifdef DEBUG.

@@ +252,5 @@
>      }
>  
>  #ifdef DEBUG
>    // Can't mark MOZ_OVERRIDE due to bug in clang builders we use for osx b2g desktop. :-(
> +  virtual void AssertOnDispatchThread() MOZ_OVERRIDE_HACKAROUND_CLANGBUG

I suggest that you do one of these things:

1. inline all the above code here, and dispense with the MOZ_OVERRIDE_HACKAROUND_CLANGBUG

2. Replace the reference to AssertOnDispatchThread in the comment above the macro definition with more generic text. Also, optionally move the macro definition to the same header the defines MOZ_OVERRIDE.
Attachment #8558009 - Flags: feedback+
dholbert - glandium said we just need to bump the clang version on our b2g osx builds and then the problem goes away. I was just in a rush (and uplifting to beta) so I didn't have time to do it.
Comment on attachment 8558009 [details] [diff] [review]
fix v1

Feedback- per the above
Attachment #8558009 - Flags: feedback-
(Assignee)

Comment 5

3 years ago
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> dholbert - glandium said we just need to bump the clang version on our b2g
> osx builds and then the problem goes away. I was just in a rush (and
> uplifting to beta) so I didn't have time to do it.

Nice, I agree we shouldn't bother with this hack then.

Is there a bug on bumping that clang version? Should I file one?
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect
> things) from comment #3)
> > dholbert - glandium said we just need to bump the clang version on our b2g
> > osx builds and then the problem goes away. I was just in a rush (and
> > uplifting to beta) so I didn't have time to do it.
> 
> Nice, I agree we shouldn't bother with this hack then.
> 
> Is there a bug on bumping that clang version? Should I file one?

Glandium would know.
Flags: needinfo?(mh+mozilla)
I haven't seen any such bug. Please feel free to file one.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 8

3 years ago
OK -- I filed bug 1129499 & needinfo'd you to add anything that I missed.
(Assignee)

Comment 9

3 years ago
Created attachment 8561137 [details] [diff] [review]
fix v2 (trivial, just annotate it as MOZ_OVERRIDE)

With bug bug 1129499 fixed, we can just mark this as MOZ_OVERRIDE. Try run to prove that:
   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10fdab84e14
Attachment #8558009 - Attachment is obsolete: true
Attachment #8561137 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Depends on: 1129499
(Assignee)

Comment 10

3 years ago
Comment on attachment 8561137 [details] [diff] [review]
fix v2 (trivial, just annotate it as MOZ_OVERRIDE)

bholley granted rs=him over IRC. Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a393700a62d
Attachment #8561137 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/6a393700a62d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.