Re: [PATCH 1/5] d3dx9_36: Implement D3DXFileCreate. (try 3)

[Christian Costa]

2012/10/24 Dmitry Timoshkov
> Christian Costa  wrote:
> > > > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface,
> > >
> > > REFIID riid, void **ret_iface)
> > > > +{
> > > > - TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid),
>
> ret_iface);
> > > > -
> > > > - if (IsEqualGUID(riid, &IID_IUnknown) ||
> > > > -
> > > >    IsEqualGUID(riid, &IID_ID3DXFile))
> > > > - {
> > > > -
> > > >    iface->lpVtbl->AddRef(iface);
> > >
> > > Isn't there an appropriate xxx_AddRef() macro?
> >
> > No.
>
> Is there a reason why?

It's not in headers as for effect interfaces.
> > > > -
> > > >    *ret_iface = iface;
> > > > -
> > > >    return S_OK;
> > > > - }
> > > > -
> > > > - ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid),
> > >
> > > ret_iface);
> > >
> > > FIXME seems more appropriate here.
> >
> > No. All interfaces are implemented here.
>
> Then ERR() is even more inappropriate here.

I'll turn it to a WARN then.
> > > > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile)
> > > > +{
> > > > - ID3DXFileImpl *object;
> > > > -
> > > > - TRACE("(%p)\n", dxfile);
> > > > -
> > > > - object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> > >
> > > sizeof(*object));
> > >
> > > What's the reason for HEAP_ZERO_MEMORY here?
> >
> > It's common to do it this way. Only non zero value are set on creation.
> > Usually several members are set in methods.
>
> There is no memebers left at 0 here.

I do it this way for all COM objects because members can be added as the
implementation advance.
In this particular case, I don't expect any new member so I will remove the
flag if it's important.
> > > > - if (!object)
> > > > - {
> > > > -
> > > >    ERR("Out of memory\n");
> > > > -
> > > >    return E_OUTOFMEMORY;
> > > > - }
> > >
> > > The ERR() is useless here, just return E_OUTOFMEMORY in such
>
> situations.
> > >
> >
> > It's done this way in many places in wine.
>
> Then that other places need to be fixed as well. Printing an ERR() on
> memory
> allocation errors is not helpful.

Well, that would have been better to spot this *much* earlier.

More generally about coding style or rules, that would be *much* better to
write a wiki page about this and point to it
along with code style comments. I don't mind following rules or adopt "nice
to do" but I cannot guess the difference
between formal rules, informal ones and personal tastes. It would be better
for everyone and focus the review
on technical stuff. And this is not the first time I talk about such a page.