[PATCH 1/2] ddraw: COM cleanup for IDirectDrawSurface3 iface

[Michael Stefaniuc]

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