586279 - Broken DeviceManagerD3D9.h compilation on mingw

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Jacek Caban]

Attachment: fix v1.0

After the patch from bug 585248, I get following error on mingw:

/home/jacek/mozilla-build/mozilla-central/gfx/layers/d3d9/DeviceManagerD3D9.h:110:3: error: extra qualification 'mozilla::layers::DeviceManagerD3D9::' on member 'AddRef'
/home/jacek/mozilla-build/mozilla-central/gfx/layers/d3d9/DeviceManagerD3D9.h:111:3: error: extra qualification 'mozilla::layers::DeviceManagerD3D9::' on member 'Release'

The error is valid and it's MSC feature that the code compiles. The problem is that AddRef and Release is implemented using NS_IMPL_ADDREF and NS_IMPL_RELEASE macros:

// We want the nsrefcnt return value. So we cannot use the inline refcnt macro
NS_IMPL_ADDREF(DeviceManagerD3D9)
NS_IMPL_RELEASE(DeviceManagerD3D9)

These macros are meant to add implementation outside the class declaration, as they use class::funcname syntax. One solution would be to move these macros to .cpp file and put only declaration here, but they are really better inline IMO. We could also add full implementation here, but I've chosen another solution. As the comment says, the reason to not use NS_INLINE_DECL_REFCOUNTING is that it needs to return refcnt so that LaoutManagerD3D9 knows if it's destroying mDeviceManager or not. It can be avoided by moving nulling mDeviceManager to DeveceManagerD3D9's destructor.

Comment 1

[Bas Schouten (:bas.schouten)]

So, the reason I don't like this is because it does not allow any class to ever decide to create its own DeviceManagerD3D9 (obviously the destructor for that class's device manager would then null out the global static. Right now we don't do this, but I'm not so certain this won't happen in the future under certain conditions, I would really like to avoid it.

I think we should either do your earlier proposed solution (obviously as you point out what I did is wrong, MSVC just seems to be tolerant of it, I should've known from the 'IMPL' part), and add a full implementation (perhaps even add Macro's for it?). An alternative would be to add a big fat comment saying that this class exposes some very unexpected behavior :-).

Comment 2

[Jacek Caban]

Thanks for the review.

(In reply to comment #1)
> So, the reason I don't like this is because it does not allow any class to ever
> decide to create its own DeviceManagerD3D9 (obviously the destructor for that
> class's device manager would then null out the global static. Right now we
> don't do this, but I'm not so certain this won't happen in the future under
> certain conditions, I would really like to avoid it.

It should work fine. That's why I check if the destroyed object is the same as stored in global static:

+      if(aDeviceManager == mDeviceManager)
+          mDeviceManager = nsnull;

So only on destruction of currently used mDeviceManager it will be nulled and it's transparent for DeviceManagerD3D9's users.

I will implement the other solution if you prefer it anyway, so please let me know what you think.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 464820 [details] [diff] [review]
fix v1.0

Hrm, I'm really torn about this! If only NS_INLINE_DECL_REFCOUNTING would just return the refcounts. But this approach certainly gets points for creativity!

Let's do this, r+. Please adjust the single line if-cases to use { } to adhere to the MDC coding style guide and the rest of layers. And change identation level of OnDeviceManagerDestroy to 2.

Comment 4

[Bas Schouten (:bas.schouten)]

Joe, I think the folks using mingw builds might want this.

Comment 5

[Joe Drew (not getting mail)]

I'll approve a patch, but this doesn't block.

Comment 6

[Jacek Caban]

Thanks for the review and approve. Pushed to m-c:

http://hg.mozilla.org/mozilla-central/rev/8da350429494