Re: d3dx8 [2/2]: add basic functions for MatrixStack

[Stefan Dösinger]

Am Mittwoch, 5. Dezember 2007 18:13:30 schrieb [e-mail]:
> This is my first attempt with ICOM objects. So, all the advices are
> welcome.

A few suggestions:

+HRESULT WINAPI ID3DXMatrixStackImpl_QueryInterface(ID3DXMatrixStack *iface, 
REFIID riid, void **ppvObject)
+ ...
+     IClassFactory_AddRef(iface);
It is better to use ID3DXMatrixStack_AddRef(iface) here. It is essentially the 
same, but it looks more consistent

The AddRef() and Release() methods should have a TRACE which write either the 
new or old refcount, otherwise tracking refcounting bugs is hell.

d3dx8 [2/2]: add basic functions for MatrixStack

[H. Verbeet]

On 05/12/2007, Stefan Dösinger  wrote:
> Am Mittwoch, 5. Dezember 2007 18:13:30 schrieb [e-mail]:
> > This is my first attempt with ICOM objects. So, all the advices are
> > welcome.
>
> A few suggestions:
>
> +HRESULT WINAPI ID3DXMatrixStackImpl_QueryInterface(ID3DXMatrixStack *iface,
> REFIID riid, void **ppvObject)
> - ...
> -
> IClassFactory_AddRef(iface);
>
> It is better to use ID3DXMatrixStack_AddRef(iface) here. It is essentially the
> same, but it looks more consistent
>
> The AddRef() and Release() methods should have a TRACE which write either the
> new or old refcount, otherwise tracking refcounting bugs is hell.

Having a TRACE in QueryInterface is occasionally useful as well.

A few more comments:
  - QueryInterface should return S_OK rather than D3D_OK, unless
you've got tests to prove it doesn't.
  - AFAIK on failure you should set *ppvObject to NULL.
  - There's no real reason to stick to MS names for parameters.

d3dx8 [2/2]: add basic functions for MatrixStack

[Stefan Dösinger]

Am Donnerstag, 6. Dezember 2007 09:38:51 schrieb H. Verbeet:
> On 05/12/2007, Stefan Dösinger  wrote:
> > Am Mittwoch, 5. Dezember 2007 18:13:30 schrieb [e-mail]:
> > > This is my first attempt with ICOM objects. So, all the advices are
> > > welcome.
> >
> > A few suggestions:
> >
> > +HRESULT WINAPI ID3DXMatrixStackImpl_QueryInterface(ID3DXMatrixStack
> > *iface, REFIID riid, void **ppvObject)
> > - ...
> > -
> > IClassFactory_AddRef(iface);
> >
> > It is better to use ID3DXMatrixStack_AddRef(iface) here. It is
> > essentially the same, but it looks more consistent
> >
> > The AddRef() and Release() methods should have a TRACE which write either
> > the new or old refcount, otherwise tracking refcounting bugs is hell.
>
> Having a TRACE in QueryInterface is occasionally useful as well.
>
> A few more comments:
> - QueryInterface should return S_OK rather than D3D_OK, unless
>
> you've got tests to prove it doesn't.

Well, that one is unprovable since D3D_OK is just S_OK :-)
> - AFAIK on failure you should set *ppvObject to NULL.
> - There's no real reason to stick to MS names for parameters.

Agreed

d3dx8 [2/2]: add basic functions for MatrixStack

[H. Verbeet]

On 06/12/2007, Stefan Dösinger  wrote:
> Well, that one is unprovable since D3D_OK is just S_OK :-)

Uh, right. The code should use S_OK then.