Re: [PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Henri Verbeet]

On 17 April 2016 at 22:57, Aaryaman Vasishta  wrote:
> @@ -1054,10 +1058,50 @@ static ULONG WINAPI d3drm3_Release(IDirect3DRM3 *iface)
>  static HRESULT WINAPI d3drm3_CreateObject(IDirect3DRM3 *iface,
>          REFCLSID clsid, IUnknown *outer, REFIID iid, void **out)
>  {
> - FIXME("iface %p, clsid %s, outer %p, iid %s, out %p stub!\n",
> - void *object_struct;

...
> - if (IsEqualGUID(clsid, &CLSID_CDirect3DRMTexture))
> - {
> -
>    if (FAILED(hr = d3drm_texture_create((struct d3drm_texture **)&object_struct)))
> -
>    {
> -
>        *out = NULL;
> -
>        return hr;
> -
>    }
> -
>    object = (IUnknown *)&((struct d3drm_texture *)object_struct)->IDirect3DRMTexture3_iface;
> - }

Why do you need "object_struct"?

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Aaryaman Vasishta]

On Mon, Apr 18, 2016 at 4:17 PM, Henri Verbeet  wrote:
> On 17 April 2016 at 22:57, Aaryaman Vasishta 
> wrote:
> > @@ -1054,10 +1058,50 @@ static ULONG WINAPI d3drm3_Release(IDirect3DRM3
>
> *iface)
> > static HRESULT WINAPI d3drm3_CreateObject(IDirect3DRM3 *iface,
> >          REFCLSID clsid, IUnknown *outer, REFIID iid, void **out)
> >  {
> > - FIXME("iface %p, clsid %s, outer %p, iid %s, out %p stub!\n",
> > - void *object_struct;
>
> ...
> > - if (IsEqualGUID(clsid, &CLSID_CDirect3DRMTexture))
> > - {
> > -
> >    if (FAILED(hr = d3drm_texture_create((struct d3drm_texture
>
> **)&object_struct)))
> > -
> >    {
> > -
> >        *out = NULL;
> > -
> >        return hr;
> > -
> >    }
> > -
> >    object = (IUnknown *)&((struct d3drm_texture
>
> *)object_struct)->IDirect3DRMTexture3_iface;
> > - }
>
> Why do you need "object_struct"?

I need it to store the object's address, from which the IUnknown interface
is obtained which in turn is used to QI the required object using iid.

Am I missing something? Is there a better way? I suppose I could re-use
object and keep it as a void * instead of IUnknown..

Cheers,
Aaryaman

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Henri Verbeet]

On 18 April 2016 at 15:15, Aaryaman Vasishta  wrote:
> On Mon, Apr 18, 2016 at 4:17 PM, Henri Verbeet  wrote:
> > Why do you need "object_struct"?
>
> I need it to store the object's address, from which the IUnknown interface
> is obtained which in turn is used to QI the required object using iid.
>
> Am I missing something? Is there a better way? I suppose I could re-use
> object and keep it as a void * instead of IUnknown..

What's wrong with using a d3drm_texture structure?

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Aaryaman Vasishta]

I thought it'd be a good idea to keep it void * for once future objects
would be implemented (i.e. the one's that have todo marked as true in the
tests).
But you do have a point. Since I'm just returning texture objects right
now, maybe using d3drm_texture structure would do. What should I go with?

Cheers,
Aaryaman

On Mon, Apr 18, 2016 at 6:58 PM, Henri Verbeet  wrote:
> On 18 April 2016 at 15:15, Aaryaman Vasishta 
> wrote:
> > On Mon, Apr 18, 2016 at 4:17 PM, Henri Verbeet
>
> wrote:
> > > Why do you need "object_struct"?
> >
> > I need it to store the object's address, from which the IUnknown
>
> interface
> > is obtained which in turn is used to QI the required object using iid.
> >
> > Am I missing something? Is there a better way? I suppose I could re-use
> > object and keep it as a void * instead of IUnknown..
>
> What's wrong with using a d3drm_texture structure?

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Henri Verbeet]

On 18 April 2016 at 17:45, Aaryaman Vasishta  wrote:
> I thought it'd be a good idea to keep it void * for once future objects
> would be implemented (i.e. the one's that have todo marked as true in the
> tests).

That can't work in a sane way. You need a specific type because you
need to dereference the pointer to get at the interface. I.e., notice
how you're casting "object_struct" in every place where you use it.

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Aaryaman Vasishta]

How about this?

if (IsEqualGUID(clsid, &CLSID_CDirect3DRMTexture))
> {
>     struct d3drm_texture *texture;
>     if (FAILED(hr = d3drm_texture_create(&texture))
>     {
>         *out = NULL;
>         return hr;
>     }
>     object = texture->IDirect3DRMTexture3_iface;
> }

What do you think? I'm not sure if declaring variables like that is
allowed, but it has been used in similar cases elsewhere so I guess it
should be fine.

On Mon, Apr 18, 2016 at 9:29 PM, Henri Verbeet  wrote:
> On 18 April 2016 at 17:45, Aaryaman Vasishta 
> wrote:
> > I thought it'd be a good idea to keep it void * for once future objects
> > would be implemented (i.e. the one's that have todo marked as true in the
> > tests).
>
> That can't work in a sane way. You need a specific type because you
> need to dereference the pointer to get at the interface. I.e., notice
> how you're casting "object_struct" in every place where you use it.

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Aaryaman Vasishta]

> > object = &texture->IDirect3DRMTexture3_iface;
>
> My bad, I forgot the & in &texture->IDirect3DRMTexture3_iface, but I'm

sure you got what I meant.

Cheers,
Aaryaman

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Henri Verbeet]

On 18 April 2016 at 18:14, Aaryaman Vasishta  wrote:
> > > object = &texture->IDirect3DRMTexture3_iface;
>
> My bad, I forgot the & in &texture->IDirect3DRMTexture3_iface, but I'm sure
> you got what I meant.

That's fine. What you'll probably want once you start supporting other
objects is a table that maps the CLSID to a creation function.

[PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject.

[Aaryaman Vasishta]

Right, I'll send another patch with the changes.

Cheers,
Aaryaman

On Mon, Apr 18, 2016 at 9:47 PM, Henri Verbeet  wrote:
> On 18 April 2016 at 18:14, Aaryaman Vasishta 
> wrote:
> > > > object = &texture->IDirect3DRMTexture3_iface;
> >
> > My bad, I forgot the & in &texture->IDirect3DRMTexture3_iface, but I'm
>
> sure
> > you got what I meant.
>
> That's fine. What you'll probably want once you start supporting other
> objects is a table that maps the CLSID to a creation function.