Re: d3drm: Fix refcounting for IDirect3DRM{1-3}

[Stefan Dösinger]

-----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.
> - 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.)
> - 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

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-----

d3drm: Fix refcounting for IDirect3DRM{1-3}

[Aaryaman Vasishta]

On Thu, Apr 30, 2015 at 1:11 PM, Stefan Dösinger 
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-----