1128576 - 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]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: 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 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 3

[Bobby Holley (:bholley)]

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 4

[Bobby Holley (:bholley)]

Comment on attachment 8558009 [details] [diff] [review]
fix v1

Feedback- per the above

Comment 5

[Daniel Holbert [:dholbert]]

(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?

Comment 6

[Bobby Holley (:bholley)]

(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.

Comment 7

[Mike Hommey [:glandium]]

I haven't seen any such bug. Please feel free to file one.

Comment 8

[Daniel Holbert [:dholbert]]

OK -- I filed bug 1129499 & needinfo'd you to add anything that I missed.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: 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

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a393700a62d