On Wednesday 25 May 2011 00:39:46 Ričardas Barkauskas wrote:
My first try for COM cleanup. Please donėt be too harsh :) If this is too large for one patch, please suggest how to split it.
This is not automatic clean up so I don't really expect it to be good on first try.
I can't find anything wrong with it on first sight, but I don't really know how to manually review a patch that large with many repetitions. I guess I could print them on a *lot* of paper and read them at the pool or something like that.
I don't see an easy way to split them up either :-/ . I guess we'll have to trust Puk's script verification.
Just one thing: in unsafe_impl_from_IDirectDrawSurface*:
- if (iface->lpVtbl != &ddraw_surface7_vtbl) return NULL;
I don't think apps can pass in their own ddraw object implementations. I've never seen an app that does that or any document that suggests this is possible and achives something. So I'd recommend to add a FIXME() or ERR() to that case. An ERR because the most likely cause for such a behavior is that our own ddrawex.dll screws up. This is the only other place where we have a IDirectDrawSurface* implementation.
Or we could just remove the check and see if we ever crash because trying to read surface implementation fields returns odd values.
On 05/25/2011 08:37 PM, Stefan Dösinger wrote:
On Wednesday 25 May 2011 00:39:46 Ričardas Barkauskas wrote:
My first try for COM cleanup. Please donėt be too harsh :) If this is too large for one patch, please suggest how to split it.
This is not automatic clean up so I don't really expect it to be good on first try.
I can't find anything wrong with it on first sight, but I don't really know how to manually review a patch that large with many repetitions. I guess I could print them on a *lot* of paper and read them at the pool or something like that.
I normally read such a patch at least 3 times, more if I find some issues and have to manually fix them. Well "read"... after hundreds of such patches it isn't really a conscious read anymore. More like optical filtering looking for odd things; that's why I *hate* inconsistent coding style.
I don't see an easy way to split them up either :-/ . I guess we'll have to trust Puk's script verification.
The patch could be split up. What I do for such big and ugly patches is to first handle the unsafe_impl_from_IFace() part, then introduce impl_from_IFace() and only afterwards do the obj->lpVtl to obj->iface changes. Of course that really depends on the sizes of the resulting patches, but most of the times the unsafe_impl_from_IFace() part is a different patch as that isn't standard search and replace cookie cutter COM cleanup.
Just one thing: in unsafe_impl_from_IDirectDrawSurface*:
- if (iface->lpVtbl !=&ddraw_surface7_vtbl) return NULL;
I don't think apps can pass in their own ddraw object implementations. I've never seen an app that does that or any document that suggests this is possible and achives something. So I'd recommend to add a FIXME() or ERR() to that case. An ERR because the most likely cause for such a behavior is that our own ddrawex.dll screws up. This is the only other place where we have a IDirectDrawSurface* implementation.
Well in that case an assert() is the better option. It's done that way in other unsafe_impl_from_IFace like functions. And the program is likely to crash anyway later on when doing magic with the wrong pointer.
Or we could just remove the check and see if we ever crash because trying to read surface implementation fields returns odd values.
bye michael
-
Michael Stefaniuc -
Stefan Dösinger