Re: [PATCH 1/4] dx8vb: Add stubs for D3DX8 interface

[Fabian Maurer]

Hi Henry,
thanks for the quick reply.
> > +typedef struct
> > +{
> > - ID3DX8 ID3DX8_iface;
> > - LONG ref;
> >
> > +} d3dx8;
>
> Please don't typedef structures just to save keystrokes.

Mind explaining the reason for this? It's like this in a lot of other places, 
and I'm curious to why you prefer it to not be a typedef.
> > +static HRESULT WINAPI d3dx8_QueryInterface(ID3DX8 *iface, REFIID riid,
> > void **ppv) +{
> > - d3dx8 *This = impl_from_ID3DX8(iface);
>
> Please use lowercase variable names.

Again, it's like this in 95% of all other cases. I'll change it if you want, I 
just wanted it to remain consistent with the existing code.
> > - TRACE("(%p/%p)->(%s,%p)\n", iface, This, debugstr_guid(riid), ppv);
> > -
> > - *ppv = NULL;
> > -
> > - if (IsEqualGUID(riid, &IID_IUnknown))
> > -
> >    *ppv = &This->ID3DX8_iface;
> > - else if(IsEqualGUID(riid, &IID_ID3DX8))
> > -
> >    *ppv = &This->ID3DX8_iface;
> > - else if(IsEqualGUID(riid, &IID_IPersistPropertyBag))
> > -
> >    FIXME("No interface for IID_IPersistPropertyBag\n");
> > - else if(IsEqualGUID(riid, &IID_IPersistStreamInit))
> > -
> >    FIXME("No interface for IID_IPersistStreamInit\n");
> > - else
> > -
> >    FIXME("No interface for %s\n", debugstr_guid(riid));
>
> Not implementing an arbitrary interface is a WARN at best.

Mind explaining why? The wiki states that "Messages in this class are meant to 
signal unimplemented features, known bugs, etc. They serve as a constant and 
active reminder of what needs to be done." This sounds fitting for me.
> > - if (*ppv)
> > - {
> > -
> >    IUnknown_AddRef((IUnknown *)(*ppv));
> > -
> >    return S_OK;
> > - }
> > -
> > - return E_NOINTERFACE;
> >
> > +}
>
> I think Nikolay already hinted at this, but this is fairly different
> from the canonical QueryInterface() implementation, especially for an
> object that only implements a single interface.

But they don't only implement one interface, they have those other GUIDs that 
are queried, no?
> > +static HRESULT WINAPI d3dx8_CreateFont(ID3DX8 *iface, Direct3DDevice8
> > *device, +          LONG hFont, D3DXFont **retFont)
>
> Please use lowercase parameter names, and avoid the stray 'h'.

I mostly just took the official names, want me to edit all of them? Wouldn't 
be an issue for me, just need to know.
> > +static HRESULT WINAPI d3dx8_AssembleShaderFromFile(ID3DX8 *iface, BSTR
> > file, LONG flags, BSTR *log, +        D3DXBuffer **constants, D3DXBuffer
> > **ppVertexShader)
> > +{
> > - FIXME("(%s, %i, %p, %p, %p): stub!\n", debugstr_w(file), flags, log,
> >
> > constants, ppVertexShader);
>
> I certainly have my preferences when it comes to trace formatting, but
> I think "%i" is questionable for flags regardless of preference.

Maybe, I'm not good with trace formatting. Is there a list for which type I 
should use what? I'd really appreciate that, maybe in the wiki? I didn't find 
anything.
> > +static const ID3DX8Vtbl ID3DX8_Vtbl =
>
> "d3dx8_vtbl"
> > +HRESULT d3dx8_create(IUnknown *outer_unk, void **ppv)
> > +{
> > - d3dx8 *object;
> > -
> > - TRACE("(%p,%p)\n", outer_unk, ppv);
> > -
> > - object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> >
> > sizeof(d3dx8));
>
> "sizeof(*object)"

I too took this from already existing code, but I'll change it.

Regards,
Fabian Maurer