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

[Henri Verbeet]

Perhaps this is sufficiently far removed from core Direct3D that
nobody cares, but for what it's worth:

On 31 October 2017 at 03:01, Fabian Maurer  wrote:
> +typedef struct
> +{
> - ID3DX8 ID3DX8_iface;
> - LONG ref;
>
> +} d3dx8;

Please don't typedef structures just to save keystrokes.
> +static HRESULT WINAPI d3dx8_QueryInterface(ID3DX8 *iface, REFIID riid, void **ppv)
> +{
> - d3dx8 *This = impl_from_ID3DX8(iface);

Please use lowercase variable names.
> -
> - 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.
> -
> - 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.
> +static HRESULT WINAPI d3dx8_CreateFont(ID3DX8 *iface, Direct3DDevice8 *device,
> -
>      LONG hFont, D3DXFont **retFont)

Please use lowercase parameter names, and avoid the stray 'h'.
> +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.
> +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)"

[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

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

[Stefan Dösinger]

Hi Fabian,

I'm not Henri, but I'll try to answer your questions:

Am 2017-10-31 um 20:48 schrieb Fabian Maurer:
> > 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.

Here I can mostly refer to the other d3d libraries (e.g. d3d8) that do
not use typedefs for structs. We had a discussion about this on
wine-devel a while ago that I could not find in the archive, but here's
e.g. the kernel's guide on this:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#typedefs .

I don't think we have an official policy, and as you correctly point out
a lot of Wine code (and Microsoft's headers) use typedefs. But since
Henri is the d3d maintainer his word counts here.

(It certainly isn't helpful that
https://wiki.winehq.org/Wine_Developer%27s_Guide/COM_in_Wine#Implementing_a_....
contains loads of old suggestions)
> > > +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.

Similarly I can only refer to ddraw/d3d8/9/10/11 here, but other places
in Wine certainly differ.
> 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.

It's largely because some applications query various (sometimes even
app-internal) interfaces and expect a failure, e.g. as a way to detect
what kind of object they got out of their internal data structures.
Printing a FIXME in QueryInterface is just as likely to mislead a user
or developer into thinking the interface should be supported as it is to
point out a bug.

If you know an interface should be supported, e.g. through tests or
because the documentation says so, but you don't implement it, that
would be a case for a FIXME.
> > > +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.

Yeah, we try to keep a distance from Microsoft's formatting as far as
possible without breaking compatibility. Renaming a parameter keeps the
code compatible to Microsoft's headers, but e.g. renaming a struct
member would break things.

This is mostly an issue for writing our own headers because it is
technically possible (but not allowed) to copypaste them from
Microsoft's headers.
> 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.

Flags are ORed bits. so %#x is the canonical way to print them. You'll
get output like 0x105 and see that there are flags 0x100, 0x004 and
0x001 set. With %i you get "261", which is much harder to read (256 | 4
| 1). (the # prints "0x105" instead of "105", which makes it clear that
it is a hexadecimal number)

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

[Fabian Maurer]

Hi Stefan,
> Here I can mostly refer to the other d3d libraries (e.g. d3d8) that do
> not use typedefs for structs. We had a discussion about this on
> wine-devel a while ago that I could not find in the archive, but here's
> e.g. the kernel's guide on this:
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#typedefs .
>
> I don't think we have an official policy, and as you correctly point out
> a lot of Wine code (and Microsoft's headers) use typedefs. But since
> Henri is the d3d maintainer his word counts here.

Thanks for the explanation, I'm fine with that. I'll rework this.
> (It certainly isn't helpful that
> https://wiki.winehq.org/Wine_Developer%27s_Guide/COM_in_Wine#Implementing_a_
> COM_interface. contains loads of old suggestions)
> > > > +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.
>
> Similarly I can only refer to ddraw/d3d8/9/10/11 here, but other places
> in Wine certainly differ.

No problem, I'll change that. It's not a problem to use words that are 
reserved words in C++ since we're using only plain C, right?
> > 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.
>
> It's largely because some applications query various (sometimes even
> app-internal) interfaces and expect a failure, e.g. as a way to detect
> what kind of object they got out of their internal data structures.
> Printing a FIXME in QueryInterface is just as likely to mislead a user
> or developer into thinking the interface should be supported as it is to
> point out a bug.
>
> If you know an interface should be supported, e.g. through tests or
> because the documentation says so, but you don't implement it, that
> would be a case for a FIXME.

Ah right, I just assume it's required since it's queried, but it makes sense 
what you say. I'll turn it in to the regular warning in these cases then.
> > > > +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.
>
> Yeah, we try to keep a distance from Microsoft's formatting as far as
> possible without breaking compatibility. Renaming a parameter keeps the
> code compatible to Microsoft's headers, but e.g. renaming a struct
> member would break things.

That's good to know, because I don't like the formatting either. I'll rewrite 
that into proper names then.
> This is mostly an issue for writing our own headers because it is
> technically possible (but not allowed) to copypaste them from
> Microsoft's headers.
> > 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.
>
> Flags are ORed bits. so %#x is the canonical way to print them. You'll
> get output like 0x105 and see that there are flags 0x100, 0x004 and
> 0x001 set. With %i you get "261", which is much harder to read (256 | 4
>
> | 1). (the # prints "0x105" instead of "105", which makes it clear that
>
> it is a hexadecimal number)

What about enums/LONG/int, is this all "%d"?

Thanks for the detailed explanations.

Regards,
Fabian Maurer

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

[Matteo Bruni]

2017-10-31 22:30 GMT+01:00 Fabian Maurer :
> > (It certainly isn't helpful that
> > https://wiki.winehq.org/Wine_Developer%27s_Guide/COM_in_Wine#Implementing_a_
> > COM_interface. contains loads of old suggestions)
> > > > > +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.
> >
> > Similarly I can only refer to ddraw/d3d8/9/10/11 here, but other places
> > in Wine certainly differ.
>
> No problem, I'll change that. It's not a problem to use words that are
> reserved words in C++ since we're using only plain C, right?

It's not a problem but in d3d-related code we tend to avoid using
"this" as the name for object variables (e.g. you would name it
"d3dx8" here, or something like that).

In general, please give a look at recent wined3d code / commits to
have an idea WRT the code style we prefer to use in d3d.

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

[Stefan Dösinger]

Am 2017-10-31 um 22:30 schrieb Fabian Maurer:
> What about enums/LONG/int, is this all "%d"?

My gut feeling says yes, although I am not entirely sure myself.

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

[Henri Verbeet]

On 1 November 2017 at 11:40, Stefan Dösinger  wrote:
> Am 2017-10-31 um 22:30 schrieb Fabian Maurer:
> > What about enums/LONG/int, is this all "%d"?
>
> My gut feeling says yes, although I am not entirely sure myself.

It depends on the usage. For enums I'd say %#x by default, but for
some a debug helper function (%s), %u or %d could make sense. For e.g.
int, %d may make sense for a display coordinate, but %#x may be more
appropriate for a file or memory offset.