Fix instances of "warning: 'mozilla::layers::[something]::SetCompositor' hides overloaded virtual function [-Woverloaded-virtual]"

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Filing bug on these two related build warnings (which I noticed when building m-i with clang 3.4 today):

First warning:
{
TextureHostOGL.h:272:8: warning: 
      'mozilla::layers::SharedTextureSourceOGL::SetCompositor' hides overloaded virtual function
      [-Woverloaded-virtual]
  void SetCompositor(CompositorOGL* aCompositor);
       ^
../../dist/include/mozilla/layers/TextureHost.h:152:16: note: hidden overloaded virtual function
      'mozilla::layers::NewTextureSource::SetCompositor' declared here: type mismatch at 1st parameter
      ('mozilla::layers::Compositor *' vs 'mozilla::layers::CompositorOGL *')
  virtual void SetCompositor(Compositor* aCompositor) {}
               ^
}

Second warning:
{
MacIOSurfaceTextureHostBasic.h:41:8: warning: 
      'mozilla::layers::MacIOSurfaceTextureSourceBasic::SetCompositor' hides overloaded virtual function
      [-Woverloaded-virtual]
  void SetCompositor(BasicCompositor* aCompositor) {
       ^
../../dist/include/mozilla/layers/TextureHost.h:152:16: note: hidden overloaded virtual function
      'mozilla::layers::NewTextureSource::SetCompositor' declared here: type mismatch at 1st parameter
      ('mozilla::layers::Compositor *' vs 'mozilla::layers::BasicCompositor *')
  virtual void SetCompositor(Compositor* aCompositor) {}
               ^
}

Based on skimming the same function implementation in other classes, I'm pretty sure these functions want to take a Compositor* (to match the superclass signaature) and static_cast it to their own custom type. (CompositorOGL* or BasicCompositor*)
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Based on skimming the same function implementation in other classes, I'm
> pretty sure these functions want to take a Compositor* (to match the
> superclass signaature) and static_cast it to their own custom type.

Actually, the other classes I was referring to appear to be subclasses of TextureHost (which also has a SetCompositor method), whereas the classes here are subclasses of NewTextureSource. So maybe the semantics are different.

Still, I'll post a strawman patch.
nical, does this make sense / is this what was intended?

(The patch fixes GrallocTextureSourceOGL, which I don't get a build warning for (maybe because it's not used on my platform?) but which I discovered by looking for other NewTextureSource subclasses.)
Attachment #8356992 - Flags: feedback?(nical.bugzilla)
Alternately, if we really do intend for these methods to take their own custom Compositor type, then maybe we should remove the (no-op) NewTextureSource method [1]?

(I tried removing the NewTextureSource method locally, but it wouldn't compile because we invoke that method on objects of (abstract) type NewTextureSource, inside of BufferTextureHost::SetCompositor().  [2]

(Though I suspect that invocation might be a no-op currently, because the NewTextureSource subclasses don't provide a SetCompositor implementation, so they just get the empty impl from NewTextureSource.)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.h?rev=033f40f8414d&mark=152-152#134
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp?rev=033f40f8414d&mark=384-386#378
Comment on attachment 8356992 [details] [diff] [review]
fix v1: make SetCompositor impls take a Compositor* and static_cast it

FWIW, I did end up finding some other NewTextureSource classes that use static_cast like "fix v1" does. They're all subclasses of DataTextureSource (which itself is a subclass of NewTextureSource):

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/TextureD3D9.cpp?rev=dd1d484f6d22#1210
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp?rev=033f40f8414d#392
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp?rev=ff328828dfda#271

So I think the approach is probably correct. Hence, upgrading feedback request to review request.
Attachment #8356992 - Flags: feedback?(nical.bugzilla) → review?(nical.bugzilla)
Comment on attachment 8356992 [details] [diff] [review]
fix v1: make SetCompositor impls take a Compositor* and static_cast it

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

You are right, this is much better.

::: gfx/layers/basic/MacIOSurfaceTextureHostBasic.h
@@ +6,5 @@
>  #ifndef MOZILLA_GFX_MACIOSURFACETEXTUREHOST_BASIC_H
>  #define MOZILLA_GFX_MACIOSURFACETEXTUREHOST_BASIC_H
>  
>  #include "mozilla/layers/TextureHostBasic.h"
> +#include "BasicCompositor.h"

nit: I would prefer to move the implemenation of SetCompositor to the .cpp and keep the forward declaration instead adding an #include in the header.
Attachment #8356992 - Flags: review?(nical.bugzilla) → review+
Depends on: 957784
Posted patch fix v2 [r=nical] (obsolete) — Splinter Review
(In reply to Nicolas Silva [:nical] from comment #5)
> nit: I would prefer to move the implemenation of SetCompositor to the .cpp
> and keep the forward declaration instead adding an #include in the header.

Addressed that in this version.

Two other things that I changed here:
 - I fixed some missing parens in my static_cast in GrallocTextureHost.cpp (which caused a compile error on B2G)
 - I fixed this bug's SetCompositor issue in one other NewTextureSource subclass: MacIOSurfaceTextureHostOGL.
Attachment #8356992 - Attachment is obsolete: true
Attachment #8357390 - Flags: review+
Try run with the final patch, except without the MacIOSurfaceTextureHostOGL tweak (which I found&addressed after starting this Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=42a4e305de80

And here's a sanity-check mac-only Try run with the MacIOSurfaceTextureHostOGL tweak as well:
 https://tbpl.mozilla.org/?tree=Try&rev=9f0f54cf2a88
Summary: Two instances of "warning: 'mozilla::layers::[something]::SetCompositor' hides overloaded virtual function [-Woverloaded-virtual]" → Fix instances of "warning: 'mozilla::layers::[something]::SetCompositor' hides overloaded virtual function [-Woverloaded-virtual]"
Updated patch to adjust the parameter's type (s/CompositorOGL/Compositor/) in its decl within MacIOSurfaceTextureHostOGL, to fix build bustage on Try:
 https://tbpl.mozilla.org/?tree=Try&rev=8bd8a70d4575
Attachment #8357390 - Attachment is obsolete: true
Attachment #8357435 - Flags: review+
Blocks: 957784
No longer depends on: 957784
https://hg.mozilla.org/mozilla-central/rev/2b150c4d305a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.