Re: [PATCH 4/5] d3dx9_36: Implement D3DXFindShaderComment (based on code from Luis Busquets)
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. I don't think lack of tests in this case should prevent integration. Tests can be added later if needed. That said, if it is really a problem, I can add some basic ones. But I don't think so.
Message du 22/02/10 13:06 De : "Henri Verbeet" A : wine-devel(a)winehq.org Copie à : Objet : Re: [PATCH 4/5] d3dx9_36: Implement D3DXFindShaderComment (based on code from Luis Busquets)
I think tests would be appropriate.
On 22 February 2010 13:43, Christian Costa <titan.costa(a)wanadoo.fr> 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.)
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(a)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.)
On 22 February 2010 15:15, Christian Costa <titan.costa(a)wanadoo.fr> wrote:
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. Sure, but that trust has to be justified. I can certainly stop looking at your patches, I have enough other stuff to do, but I doubt that's going to speed up getting them accepted. It's perhaps also worth noting that I've fixed plenty of bugs that could have easily been avoided if the original author of the code had been more careful, or had bothered to write tests.
Seriously, the general principle is that you add a test when you implement some new functionality or fix a bug, unless that's not feasible for some reason. That's certainly not the case here, writing tests for this function is trivial.
Henri Verbeet a écrit :
On 22 February 2010 15:15, Christian Costa <titan.costa(a)wanadoo.fr> wrote:
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.
Sure, but that trust has to be justified. I can certainly stop looking at your patches, I have enough other stuff to do, but I doubt that's going to speed up getting them accepted. It's perhaps also worth noting that I've fixed plenty of bugs that could have easily been avoided if the original author of the code had been more careful, or had bothered to write tests.
Seriously, the general principle is that you add a test when you implement some new functionality or fix a bug, unless that's not feasible for some reason. That's certainly not the case here, writing tests for this function is trivial.
The goal was to implement the IConstantTable interface. I intended to write tests at this level along with new patch series for this interface. D3DXFindShaderComment was not needed directly by a particular app but just to be reused by D3DXGetShaderConstantTableEx as the next patch shows. But since it's better to do it in this patch, I will add some tests to it then.
participants (2)
-
Christian Costa -
Henri Verbeet