Closed Bug 586279 Opened 10 years ago Closed 10 years ago

Broken DeviceManagerD3D9.h compilation on mingw

Categories

(Core :: GFX: Color Management, defect)

x86_64
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(1 file)

Attached patch fix v1.0Splinter Review
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.
Blocks: 585248
Attachment #464820 - Attachment is patch: true
Attachment #464820 - Flags: review?(vladimir)
Summary: Broken DeviceManagerD3D9.h on mingw → Broken DeviceManagerD3D9.h compilation on mingw
Attachment #464820 - Flags: review?(vladimir) → review?(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 :-).
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.
Assignee: nobody → jacek
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.
Attachment #464820 - Flags: review?(bas.schouten) → review+
Joe, I think the folks using mingw builds might want this.
blocking2.0: --- → ?
I'll approve a patch, but this doesn't block.
blocking2.0: ? → -
Attachment #464820 - Flags: approval2.0?
Attachment #464820 - Flags: approval2.0? → approval2.0+
Thanks for the review and approve. Pushed to m-c:

http://hg.mozilla.org/mozilla-central/rev/8da350429494
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.