[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: d3drm: Fix refcounting for IDirect3DRM{1-3}
From: Aaryaman Vasishta <jem456.vasishta () gmail ! com>
Date: 2015-04-30 8:12:24
Message-ID: CABVHfRvOMUWQoVnVckj-F+=D_nViKi9ZLNYR34WpO6cqYXcY0A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Thu, Apr 30, 2015 at 1:11 PM, Stefan Dösinger <stefandoesinger@gmail.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 2015-04-29 um 23:47 schrieb Aaryaman Vasishta:
> > - if (!refcount)
> > + if (!refcount && !InterlockedDecrement(&d3drm->numIfaces))
> > HeapFree(GetProcessHeap(), 0, d3drm);
> This works, but this way you have the code that destroys the object 3
> times. Not much of an issue as long as it is just a HeapFree, but I
> think it would be nicer to have a separate function. Either add a
> d3drm_destroy function or a pair of d3drm_add_iface and
> d3drm_release_iface functions that also take care of
> InterlockedIncrement / Decrement for iface_count)
>
> > - { &IID_IDirect3DRM3, &IID_IDirect3DRM3,
> S_OK, TRUE },
> > - { &IID_IDirect3DRM2, &IID_IDirect3DRM2,
> S_OK, TRUE },
> > + { &IID_IDirect3DRM3, &IID_IDirect3DRM3,
> S_OK, FALSE },
> > + { &IID_IDirect3DRM2, &IID_IDirect3DRM2,
> S_OK, FALSE },
> Afaics all the TODOs are gone, so you can remove this field from the
> table and the todo_wine code from test_qi(). Either in the same patch
> or a separate one.
>
I was thinking to remove the TODO's in a separate patch, as both the frame
tests and d3drm tests use it. Is that okay? Hope AJ agrees with this change
:)
>
> > - LONG ref;
> > + LONG ref1, ref2, ref3, numIfaces;
> > };
> Please use iface_count. Yeah, it's just a plain style thing, but d3drm
> is nicely consistent so far in not using CamelCase (except in cases
> where Microsoft determined the name).
>
> (Yesterday on IRC I said num_ifaces. Henri says xxx_count is the way
> he prefers.)
>
I don't like CamelCase either, glad to use iface_count :)
>
> > + if (refcount == 1) InterlockedIncrement(&d3drm->numIfaces);
> Another minor style issue: Please put the statement on a separate
> line, in consistency with the existing code.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
I'll make the changes and resend the patch as try-2
Thank you!
Jam
>
> iQIcBAEBAgAGBQJVQdzBAAoJEN0/YqbEcdMw5WAP/0H52sqgEXMdSmh6sXH/dZMQ
> nTZY9uRhElUbb3r6D6HG0ylit994pPaq887t9BSl8F4vbjsPXLjruDbVSwoScCfp
> rIDKy9xET+ek4p5TSFC9cX1BFAiC2VKN7YYWmr/wHKVPZc+aFudU9ykac02bi9p2
> Oe+9wMBcFbwv33UhNL+2dugA4+0yj6egfPJWmju9C6ff6CPGqgXH70rHsrSf1AYs
> VkNPRhGpfI9BxojBChVtgc0zwPM1HnfOwfmKWCW75YNgNg9lL0o+bdoy2wCcxiLq
> FYwLLT2o9K+RnPQqRDhdJBKWwY8VrJI/fHZEURRau7RW1ia26j4UrrsAdXTQwhPs
> Iy8A63pYOxyZsphq5KJ8cwGABYIlBPUG0lXZQRgKh1Jw8f39HIR5cfnu1at/fPSc
> Cw6tfL/t+g4KeHaZDWZnROwyp704kIZGgvSaPuJjwkPtHh2MN14cp9KQj389kCUM
> 4KfV6FxsAByRuJ8ur1HSaV2OeIOKOJkaDTzaiiMqc581A0awOeCy5SuAUSrQu4IE
> 4gAIjryIJXd+kTDw46y6OW5UNXHdNJKGWAMiih25yLBi0BLBfUiW0U7qW5Lcerls
> e7zefeUYNT+Umj3WU4sDu6B8B6auSZgKrLu9JAwYH2RUhKX9R+z3uTY25VUbJkoP
> 9WJfJJuosIA5zqLbjxu+
> =/BI/
> -----END PGP SIGNATURE-----
>
[Attachment #5 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr \
30, 2015 at 1:11 PM, Stefan Dösinger <span dir="ltr"><<a \
href="mailto:stefandoesinger@gmail.com" \
target="_blank">stefandoesinger@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">-----BEGIN PGP SIGNED \
MESSAGE-----<br>
Hash: SHA1<br>
<span class=""><br>
Am 2015-04-29 um 23:47 schrieb Aaryaman Vasishta:<br>
> - if (!refcount)<br>
> + if (!refcount && \
!InterlockedDecrement(&d3drm->numIfaces))<br> > \
HeapFree(GetProcessHeap(), 0, d3drm);<br> </span>This works, but this way you have \
the code that destroys the object 3<br> times. Not much of an issue as long as it is \
just a HeapFree, but I<br> think it would be nicer to have a separate function. \
Either add a<br> d3drm_destroy function or a pair of d3drm_add_iface and<br>
d3drm_release_iface functions that also take care of<br>
InterlockedIncrement / Decrement for iface_count)<br>
<span class=""><br>
> - { &IID_IDirect3DRM3, \
&IID_IDirect3DRM3, S_OK, TRUE \
},<br> > - { &IID_IDirect3DRM2, \
&IID_IDirect3DRM2, S_OK, TRUE \
},<br> > + { &IID_IDirect3DRM3, \
&IID_IDirect3DRM3, S_OK, FALSE \
},<br> > + { &IID_IDirect3DRM2, \
&IID_IDirect3DRM2, S_OK, FALSE \
},<br> </span>Afaics all the TODOs are gone, so you can remove this field from \
the<br> table and the todo_wine code from test_qi(). Either in the same patch<br>
or a separate one.<br></blockquote><div>I was thinking to remove the TODO's in a \
separate patch, as both the frame tests and d3drm tests use it. Is that okay? Hope AJ \
agrees with this change :)<br></div><blockquote class="gmail_quote" style="margin:0px \
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span \
class=""><br> > - LONG ref;<br>
> + LONG ref1, ref2, ref3, numIfaces;<br>
> };<br>
</span>Please use iface_count. Yeah, it's just a plain style thing, but d3drm<br>
is nicely consistent so far in not using CamelCase (except in cases<br>
where Microsoft determined the name).<br></blockquote><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> <br>
(Yesterday on IRC I said num_ifaces. Henri says xxx_count is the way<br>
he prefers.)<br></blockquote><div><div>I don't like CamelCase either, glad to use \
iface_count :) <br></div> </div><blockquote class="gmail_quote" style="margin:0px \
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span \
class=""><br> > + if (refcount == 1) \
InterlockedIncrement(&d3drm->numIfaces);<br> </span>Another minor style issue: \
Please put the statement on a separate<br> line, in consistency with the existing \
code.<br>
-----BEGIN PGP SIGNATURE-----<br>
Version: GnuPG v2<br></blockquote><div>I'll make the changes and resend the patch \
as try-2<br><br></div><div>Thank you!<br></div><div>Jam <br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> <br>
iQIcBAEBAgAGBQJVQdzBAAoJEN0/YqbEcdMw5WAP/0H52sqgEXMdSmh6sXH/dZMQ<br>
nTZY9uRhElUbb3r6D6HG0ylit994pPaq887t9BSl8F4vbjsPXLjruDbVSwoScCfp<br>
rIDKy9xET+ek4p5TSFC9cX1BFAiC2VKN7YYWmr/wHKVPZc+aFudU9ykac02bi9p2<br>
Oe+9wMBcFbwv33UhNL+2dugA4+0yj6egfPJWmju9C6ff6CPGqgXH70rHsrSf1AYs<br>
VkNPRhGpfI9BxojBChVtgc0zwPM1HnfOwfmKWCW75YNgNg9lL0o+bdoy2wCcxiLq<br>
FYwLLT2o9K+RnPQqRDhdJBKWwY8VrJI/fHZEURRau7RW1ia26j4UrrsAdXTQwhPs<br>
Iy8A63pYOxyZsphq5KJ8cwGABYIlBPUG0lXZQRgKh1Jw8f39HIR5cfnu1at/fPSc<br>
Cw6tfL/t+g4KeHaZDWZnROwyp704kIZGgvSaPuJjwkPtHh2MN14cp9KQj389kCUM<br>
4KfV6FxsAByRuJ8ur1HSaV2OeIOKOJkaDTzaiiMqc581A0awOeCy5SuAUSrQu4IE<br>
4gAIjryIJXd+kTDw46y6OW5UNXHdNJKGWAMiih25yLBi0BLBfUiW0U7qW5Lcerls<br>
e7zefeUYNT+Umj3WU4sDu6B8B6auSZgKrLu9JAwYH2RUhKX9R+z3uTY25VUbJkoP<br>
9WJfJJuosIA5zqLbjxu+<br>
=/BI/<br>
-----END PGP SIGNATURE-----<br>
</blockquote></div><br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic