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

[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?
> -
>    *ret_iface = iface;
> -
>    return S_OK;
> - }
> -
> - ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid), ret_iface);

FIXME seems more appropriate here.
> - *ret_iface = NULL;
> - return E_NOINTERFACE;
>
> +}

...
> +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?
> - if (!object)
> - {
> -
>    ERR("Out of memory\n");
> -
>    return E_OUTOFMEMORY;
> - }

The ERR() is useless here, just return E_OUTOFMEMORY in such situations.

[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.
> > -
> >    *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.
> > - *ret_iface = NULL;
> > - return E_NOINTERFACE;
> >
> > +}
>
> ...
> > +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.
> > - 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.

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

[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?
> > > -
> > >    *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.
> > > +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.
> > > - 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.

[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.

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

[Dmitry Timoshkov]

Christian Costa  wrote:
> 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.

Handling out of memory errors, avoiding useless spam to the console, avoiding
blanket memset() calls are not the coding style things and they are not specific
to Wine or any other project, this is basic things a programmers needs to follow
in general IMHO.

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

[Christian Costa]

Le 24/10/2012 17:53, Dmitry Timoshkov a écrit :
> Christian Costa  wrote:
> > 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.
>
> Handling out of memory errors, avoiding useless spam to the console, avoiding
> blanket memset() calls are not the coding style things and they are not specific
> to Wine or any other project, this is basic things a programmers needs to follow
> in general IMHO.

I said "more generally". It doesn't apply particularly to this but usual 
comments
we have on the list : indentation, tabs, LPxxx, Microsoft variable 
naming, COM, debugstr_a,
sizeof, ...
I proposed to do such a page but people need to agree on the principle 
and reviewers to use
it to point to it along with comments. This way reviewers can more 
concentrate on technical
stuff. This also help people cleanup the code when there are sending patch.

If I agree with out of memory error, regarding the memset it is 
particular to COM and the incremental
nature of Wine.
In addition not all people are good programmers nor usual Wine 
developper? Note that for some goto is
evil and should never used.
If we have some tips or rules based on Wine code practical experience 
that would help patch submission.

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

[Rico Schüller]

On 24.10.2012 16:33, Dmitry Timoshkov wrote:
> 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?

Yes, they are not in the headers which are supplied by the SDK (at least 
not in the one I checked). I guess you have to ask ms why they didn't 
add it. So I think we shouldn't add that in our headers.
> > > > - 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.

In which cases do you use ERR? I thought it is used, when there are 
system errors. And I think ERR is fine for out of memory, because the 
system is not able to hand out the needed memory. Just do a git grep. 
There are ~400 usages for ERR and out of memory... What's your 
suggestion how to do that with the "Out of memory"?

Cheers
Rico

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

[Dmitry Timoshkov]

Rico Schüller  wrote:
> > > > > - 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.
>
> In which cases do you use ERR? I thought it is used, when there are 
> system errors. And I think ERR is fine for out of memory, because the 
> system is not able to hand out the needed memory. Just do a git grep. 
> There are ~400 usages for ERR and out of memory... What's your 
> suggestion how to do that with the "Out of memory"?

A much more illustrative thing that happens after an out of memory
condition is a crash followed by a backtrace (if the caller does not
handle it gracefully), printing an ERR() is completely useless in that
case, and provides no useful information.

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

[Rico Schüller]

On 24.10.2012 18:18, Dmitry Timoshkov wrote:
> Rico Schüller  wrote:
> > > > > > - 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.
> >
> > In which cases do you use ERR? I thought it is used, when there are
> > system errors. And I think ERR is fine for out of memory, because the
> > system is not able to hand out the needed memory. Just do a git grep.
> > There are ~400 usages for ERR and out of memory... What's your
> > suggestion how to do that with the "Out of memory"?
>
> A much more illustrative thing that happens after an out of memory
> condition is a crash followed by a backtrace (if the caller does not
> handle it gracefully), printing an ERR() is completely useless in that
> case, and provides no useful information.

Well the app may initialize some resources (some of them fail with out 
of memory) and run half an hour. If it accesses the invalid pointer it 
crashes. Do you see the problem from the backtrace? The real problem 
happened half an hour ago and the err message doesn't hurt anyone, it 
just shows the problem as early as possible, which I think is better 
than to relay on a eventually crashing app at some point. The err 
doesn't spam the console, since it only happens when there is a problem.

Cheers
Rico

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

[Dmitry Timoshkov]

Rico Schüller  wrote:
> Well the app may initialize some resources (some of them fail with out 
> of memory) and run half an hour. If it accesses the invalid pointer it 
> crashes. Do you see the problem from the backtrace? The real problem 
> happened half an hour ago and the err message doesn't hurt anyone, it 
> just shows the problem as early as possible, which I think is better 
> than to relay on a eventually crashing app at some point. The err 
> doesn't spam the console, since it only happens when there is a problem.

If you allocate a large chunk of memory it's probably justified to print
a WARN(), but when allocating a structure of a couple tens of bytes it's
already too late. Inside a library (and Wine is a library) printing anything
to the console should be avoided as much as possible with an obvious exception
for not implemented things.

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

[Max TenEyck Woodbury]

On 10/24/2012 12:02 PM, Rico Schüller wrote:
> On 24.10.2012 16:33, Dmitry Timoshkov wrote:
> > Christian Costa  wrote:
> > > > > - 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.
>
> In which cases do you use ERR? I thought it is used, when there are
> system errors. And I think ERR is fine for out of memory, because the
> system is not able to hand out the needed memory. Just do a git grep.
> There are ~400 usages for ERR and out of memory... What's your
> suggestion how to do that with the "Out of memory"?

My understanding is that this is similar (although less severe) than
writing log messages when out of disk space.

The report may not be generated because the resources needed to produce
the report are not available.

The absence of the report may be taken to mean that the problem is on
some other code path.

Deliberately not reporting 'out of whatever' errors or, better yet,
commenting that reporting such errors are not being generated may make
diagnosing such problems easier.

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

[Nikolay Sivov]

On 10/24/2012 19:02, Rico Schüller wrote:
> On 24.10.2012 16:33, Dmitry Timoshkov wrote:
> > 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?
>
> Yes, they are not in the headers which are supplied by the SDK (at 
> least not in the one I checked). I guess you have to ask ms why they 
> didn't add it. So I think we shouldn't add that in our headers.

Actually I don't see why we can't add an .idl for it and let widl 
produce a header. It still will be accessible directly through vtbl 
pointer if someone feels like build with our headers. Header we're 
talking about d3dx9xof.h is small enough to be a first converted to idl 
imho, instead of adding more code that uses in SDK way. As an example we 
have dwrite.idl now that SDK does not provide.

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

[Christian Costa]

Le 24/10/2012 19:39, Nikolay Sivov a écrit :
> On 10/24/2012 19:02, Rico Schüller wrote:
> > On 24.10.2012 16:33, Dmitry Timoshkov wrote:
> > > 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?
> >
> > Yes, they are not in the headers which are supplied by the SDK (at 
> > least not in the one I checked). I guess you have to ask ms why they 
> > didn't add it. So I think we shouldn't add that in our headers.
>
> Actually I don't see why we can't add an .idl for it and let widl 
> produce a header. It still will be accessible directly through vtbl 
> pointer if someone feels like build with our headers. Header we're 
> talking about d3dx9xof.h is small enough to be a first converted to 
> idl imho, instead of adding more code that uses in SDK way. As an 
> example we have dwrite.idl now that SDK does not provide.

I'm not against this if this is encouraged but I would left it for 
another patch.

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

[Rico Schüller]

On 24.10.2012 19:39, Nikolay Sivov wrote:
> On 10/24/2012 19:02, Rico Schüller wrote:
> > On 24.10.2012 16:33, Dmitry Timoshkov wrote:
> > > 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?
> >
> > Yes, they are not in the headers which are supplied by the SDK (at
> > least not in the one I checked). I guess you have to ask ms why they
> > didn't add it. So I think we shouldn't add that in our headers.
>
> Actually I don't see why we can't add an .idl for it and let widl
> produce a header. It still will be accessible directly through vtbl
> pointer if someone feels like build with our headers. Header we're
> talking about d3dx9xof.h is small enough to be a first converted to idl
> imho, instead of adding more code that uses in SDK way. As an example we
> have dwrite.idl now that SDK does not provide.

I have nothing against it, we just need to agree on something.

Last time 
(http://www.winehq.org/pipermail/wine-devel/2010-June/084633.html) it 
was also criticized ... Do we care that some of our code eventually may 
be built with dxsdk headers? (That won't work for all dlls, but for 
some). How far do we go with compatibility?

Cheers
Rico