Closed
Bug 957499
Opened 7 years ago
Closed 7 years ago
Fix instances of "warning: 'mozilla::layers::[something]::SetCompositor' hides overloaded virtual function [-Woverloaded-virtual]"
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
6.26 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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*)
Assignee | ||
Comment 1•7 years ago
|
||
(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.
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
(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+
Assignee | ||
Comment 7•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
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]"
Assignee | ||
Comment 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b150c4d305a
Flags: in-testsuite-
Assignee | ||
Updated•7 years ago
|
https://hg.mozilla.org/mozilla-central/rev/2b150c4d305a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•7 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•