[PATCH 1/4] dx8vb: Add stubs for D3DX8 interface
Fabian Maurer
dark.shadow4 at web.de
Tue Oct 31 14:48:15 CDT 2017
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
More information about the wine-devel
mailing list