<br />Wrt the correctness of the behaviour about these parameters, it seems the documentation is not<br />something we should rely on. That's why it's better to write test. So until there is an app which rely<br />on this particular behaviour, this does not worth the trouble.<br />It's not from me. This is something we can hear often on wine devel.<br /><br />Thanks for the review. I will update my patch accordingly.<br /><br />Wrt the naming conventions, Alexandre already complains about msdn style.<br />Unfortunately about the p prefix, it's not clearly stated.<br /><br />More generally I would say that over-reviewing is not a good thing. It does not bring much in term of<br />quality and slow down development. Thrust is the base of a community.<br />Just for the history, I just started doing some stuff on this dll and was surprised that many code<br />have been written which didn't get in.<br />I make a big difference between a self contained dll for which the native one works and another.<br />I make also a big difference between modifying a working code and writing code from scratch.<br /><br />That said. There is not need for a flame war or whatever.<br /> <br /> Christian<br /> <br /><blockquote style="padding-left: 5px; margin-left: 5px; border-left: 2px solid #ff0000">&gt; Message du 22/02/10 14:20<br />&gt; De : &quot;Henri Verbeet&quot; <br />&gt; A : &quot;Christian Costa&quot; <br />&gt; Copie &agrave; : wine-devel@winehq.org<br />&gt; Objet : Re: [PATCH 4/5] d3dx9_36: Implement D3DXFindShaderComment (based on         code from Luis Busquets)<br />&gt; <br />&gt; <br />&gt; On 22 February 2010 13:43, Christian Costa  wrote:<br />&gt; &gt;<br />&gt; &gt; Sure. Having tests is better but in this case there is no chance for<br />&gt; &gt; regression.<br />&gt; &gt; I usually write tests for my personnal pursose or to compare behaviour<br />&gt; &gt; against real windows or native dll.<br />&gt; You write tests to prove your implementation is correct, and to<br />&gt; prevent it from breaking in the future.<br />&gt; <br />&gt; The first thing that looks suspicious is that the documentation claims<br />&gt; &quot;ppData&quot; and &quot;pSizeInBytes&quot; are allowed to be NULL, while your<br />&gt; implementation doesn't allow that. The other thing that looks<br />&gt; immediately suspicious is that your return value for &quot;pSizeInBytes&quot; is<br />&gt; inconsistent with the size of the returned data. In terms of style,<br />&gt; you calculate the comment size twice, while you could just as easily<br />&gt; store it in a variable. Casting to LPCVOID is superfluous.<br />&gt; &quot;*pSizeInBytes&quot; is unsigned, so should use &quot;%u&quot; in the format string.<br />&gt; (I also have some reservations about your naming conventions, but if<br />&gt; you can get them past Alexandre they're mostly your own problem.)<br />&gt; <br />&gt; <br />&gt; <br />&gt; <br /></blockquote>