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