[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.
From:       Aaryaman Vasishta <jem456.vasishta () gmail ! com>
Date:       2016-06-30 11:27:20
Message-ID: CABVHfRu_6PC7MZFTRYwYyOq06aagPw9s9GAoi2_1o=VJRhbc-g () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]


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

[Attachment #5 (text/html)]

<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun \
30, 2016 at 3:41 PM, Henri Verbeet <span dir="ltr">&lt;<a \
href="mailto:hverbeet@gmail.com" target="_blank">hverbeet@gmail.com</a>&gt;</span> \
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"><span><br> </span>You can probably fix the immediate \
issue just by storing the return<br> value and releasing the reference if the QI \
failed.<br> <br>
The larger issue is that code becomes much easier to follow when you<br>
can assume that functions are supposed to leave things (more or less)<br>
the way they found them on failure. (Unless explicitly indicated.)<br>
I.e., when d3drm_device_set_ddraw_device_d3d() releases all the<br>
references it created when it fails, and d3drm1_CreateDeviceFromD3D()<br>
can assume the device is in the same state as after calling<br>
d3drm_device_create().<br></blockquote><div>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?<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
Of course the even larger issue is that<br>
d3drm_device_set_ddraw_device_d3d() doesn&#39;t make a lot of sense. In a<br>
way similar to e.g. d3drm3_CreateTexture(),<br>
d3drm1_CreateDeviceFromD3D() should probably just call<br>
IDirect3DRMDevice::InitFromD3D().<br></blockquote><div>Agreed. Back then it \
wasn&#39;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.<br></div><div>Thanks for the \
review!<br></div><div><br></div><div>Cheers,<br></div><div>Aaryaman \
<br></div></div><br></div></div>


[Attachment #6 (text/plain)]




[prev in list] [next in list] [prev in thread] [next in thread]