<br><br><div class="gmail_quote">2012/10/24 Dmitry Timoshkov <span dir="ltr"><<a href="mailto:dmitry@baikal.ru" target="_blank">dmitry@baikal.ru</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Christian Costa <<a href="mailto:titan.costa@gmail.com">titan.costa@gmail.com</a>> wrote:<br>
<br>
> > > +static HRESULT WINAPI ID3DXFileImpl_QueryInterface(ID3DXFile *iface,<br>
> > REFIID riid, void **ret_iface)<br>
> > > +{<br>
> > > +    TRACE("(%p)->(%s, %p)\n", iface, debugstr_guid(riid), ret_iface);<br>
> > > +<br>
> > > +    if (IsEqualGUID(riid, &IID_IUnknown) ||<br>
> > > +        IsEqualGUID(riid, &IID_ID3DXFile))<br>
> > > +    {<br>
> > > +        iface->lpVtbl->AddRef(iface);<br>
> ><br>
> > Isn't there an appropriate xxx_AddRef() macro?<br>
> ><br>
><br>
> No.<br>
<br>
</div>Is there a reason why?<br></blockquote><div><br></div><div>It's not in headers as for effect interfaces. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><br>
> > > +        *ret_iface = iface;<br>
> > > +        return S_OK;<br>
> > > +    }<br>
> > > +<br>
> > > +    ERR("(%p)->(%s, %p), not found\n", iface, debugstr_guid(riid),<br>
> > ret_iface);<br>
> ><br>
> > FIXME seems more appropriate here.<br>
> ><br>
><br>
> No. All interfaces are implemented here.<br>
<br>
</div>Then ERR() is even more inappropriate here.<br></blockquote><div><br></div><div>I'll turn it to a WARN then.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><br>
> > > +HRESULT WINAPI D3DXFileCreate(ID3DXFile **dxfile)<br>
> > > +{<br>
> > > +    ID3DXFileImpl *object;<br>
> > > +<br>
> > > +    TRACE("(%p)\n", dxfile);<br>
> > > +<br>
> > > +    object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,<br>
> > sizeof(*object));<br>
> ><br>
> > What's the reason for HEAP_ZERO_MEMORY here?<br>
> ><br>
><br>
> It's common to do it this way. Only non zero value are set on creation.<br>
> Usually several members are set in methods.<br>
<br>
</div>There is no memebers left at 0 here.<br></blockquote><div><br></div><div>I do it this way for all COM objects because members can be added as the implementation advance.</div><div>In this particular case, I don't expect any new member so I will remove the flag if it's important.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> > > +    if (!object)<br>
> > > +    {<br>
> > > +        ERR("Out of memory\n");<br>
> > > +        return E_OUTOFMEMORY;<br>
> > > +    }<br>
> ><br>
> > The ERR() is useless here, just return E_OUTOFMEMORY in such situations.<br>
> ><br>
> ><br>
> ><br>
> It's done this way in many places in wine.<br>
<br>
</div>Then that other places need to be fixed as well. Printing an ERR() on memory<br>
allocation errors is not helpful.<br>
<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div> </div><div>Well, that would have been better to spot this *much* earlier.</div><div><br></div><div>More generally about coding style or rules, that would be *much* better to write a wiki page about this and point to it</div>
<div>along with code style comments. I don't mind following rules or adopt "nice to do" but I cannot guess the difference</div><div>between formal rules, informal ones and personal tastes. It would be better for everyone and focus the review</div>
<div>on technical stuff. And this is not the first time I talk about such a page.</div></div>