On Thu, Jun 30, 2016 at 3:41 PM, Henri Verbeet <hverbeet@gmail.com> wrote:

You can probably fix the immediate issue just by storing the return
value and releasing the reference if the QI failed.

The larger issue is that code becomes much easier to follow when you
can assume that functions are supposed to leave things (more or less)
the way they found them on failure. (Unless explicitly indicated.)
I.e., when d3drm_device_set_ddraw_device_d3d() releases all the
references it created when it fails, and d3drm1_CreateDeviceFromD3D()
can assume the device is in the same state as after calling
d3drm_device_create().

So I could e.g. release the reference, to ddraw, d3drm and d3d_device and set them all to NULL, effectively putting them in the same state as it it weas before calling d3drm_device_set_ddraw_device_d3d(). Is this fine?

Of course the even larger issue is that
d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a
way similar to e.g. d3drm3_CreateTexture(),
d3drm1_CreateDeviceFromD3D() should probably just call
IDirect3DRMDevice::InitFromD3D().

Agreed. Back then it wasn't clear how CreateObject and Init worked together, though after the texture and viewport implementations, it became increasingly apparent that Init works only via CreateObject. A series of patches to move the implementations to Init could be done later on.

Thanks for the review!

Cheers,

Aaryaman