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