Re: d3dx9_36 [try 3]: Implement D3DXSHRotate
I agree with Rico. It's hard to find issues in your own code, which makes it all the more important to check it thoroughly. Still, at least try to be careful with the code style: +static void RotateX(FLOAT *out, UINT order, FLOAT a, FLOAT *in) I think it's better to only use lower case for the (private) function name. Something like rotatex or rotate_x or whatever. I can agree with the rotate name, but not with the X. X in capital is there to mean the X-axis. In math, one never uses a lower case letter for an axis. So, it would become hard to understand what means x. + if ( order 2 ) + return; Please indent it correctly. OK. + return; +} Unneeded. One always said to me that it is a good pratice that a function returns something. +FLOAT* WINAPI D3DXSHRotate(FLOAT *out, UINT order, CONST D3DXMATRIX *matrix, CONST FLOAT *in) Fix the FLOAT*. I don't understand. + return; +} Again. Also, the patch is full of trailing whitespaces, please fix those too. OK Nozomi
Re: d3dx9_36 [try 3]: Implement D3DXSHRotate
Your mail makes me thing two or three remarks. I agree that in the plain wine, code must be optimized to use less memory as possible and to be the fastest as possible. But, the tests are here to check that the implemented function behaves like the windows one. So, they don't need to be optimized. So, what is the point to worry about efficiency? It is not important that I allocate 100 floats for my array even if I use 49 only. In the same idea, the time in the loop is not important. It is the same if I spend 1s in a loop or 1/100s. They are there for testing. Tests work, it is OK. I think function is optimized in plain wine, so I can send the patch. Tests don't work. I must figure out why. About coding style, I can not understand the policy. If I look in the d3dx9_36/math.c file, I can see almost as much styles as functions. So, complaining about the style I use is a non-sense for me. Nozomi De : Rico Schüller kgbric...@web.de À : wine-devel@winehq.org; Nozomi Kodama nozomi.kod...@yahoo.com Envoyé le : Mardi 4 septembre 2012 2h44 Objet : Re: d3dx9_36 [try 3]: Implement D3DXSHRotate On 04.09.2012 13:19, Nozomi Kodama wrote: + FLOAT expected, in[100], out[100], ... + for (i = 0; i 49; i++) + in[i] = i + 1.01f; I guess this belongs together. I'd like to make some suggestions, please don't take it personally. When you got a comment on a patch, read it carefully. Think about it, if it makes sens to do the change (just adding what I've said is not always the best, it just gives you a hint, that you may improve some things, the list is not always complete and different people may have different suggestions) and try to improve your patch. After you are done, read your patch. Well mistakes happen, especially when you try to get something done really fast. (1) Take you some time and read it again and see if you've done that as best as possible and if you are happy with the code you've done, then submit it. If you've found a bug don't submit, fix it and go to #1. The earlier a bug is found the cheaper is it to solve it. I hope this helps a bit and it hopefully saves you some time to get a patch in the feature accepted. Thought I'd like to thank you for your patience and hopefully I haven't annoyed you and we'll see some good quality patches in the feature from you. :-) Cheers Rico
Re: d3dx9_36 [try 3]: Implement D3DXSHRotate
On 04.09.2012 13:19, Nozomi Kodama wrote: +FLOAT expected, in[100], out[100], ... +for (i = 0; i 49; i++) +in[i] = i + 1.01f; I guess this belongs together. I'd like to make some suggestions, please don't take it personally. When you got a comment on a patch, read it carefully. Think about it, if it makes sens to do the change (just adding what I've said is not always the best, it just gives you a hint, that you may improve some things, the list is not always complete and different people may have different suggestions) and try to improve your patch. After you are done, read your patch. Well mistakes happen, especially when you try to get something done really fast. (1) Take you some time and read it again and see if you've done that as best as possible and if you are happy with the code you've done, then submit it. If you've found a bug don't submit, fix it and go to #1. The earlier a bug is found the cheaper is it to solve it. I hope this helps a bit and it hopefully saves you some time to get a patch in the feature accepted. Thought I'd like to thank you for your patience and hopefully I haven't annoyed you and we'll see some good quality patches in the feature from you. :-) Cheers Rico
Re: d3dx9_36 [try 3]: Implement D3DXSHRotate
2012/9/4 Rico Schüller kgbric...@web.de: On 04.09.2012 13:19, Nozomi Kodama wrote: +FLOAT expected, in[100], out[100], ... +for (i = 0; i 49; i++) +in[i] = i + 1.01f; I guess this belongs together. I'd like to make some suggestions, please don't take it personally. When you got a comment on a patch, read it carefully. Think about it, if it makes sens to do the change (just adding what I've said is not always the best, it just gives you a hint, that you may improve some things, the list is not always complete and different people may have different suggestions) and try to improve your patch. After you are done, read your patch. Well mistakes happen, especially when you try to get something done really fast. (1) Take you some time and read it again and see if you've done that as best as possible and if you are happy with the code you've done, then submit it. If you've found a bug don't submit, fix it and go to #1. The earlier a bug is found the cheaper is it to solve it. I hope this helps a bit and it hopefully saves you some time to get a patch in the feature accepted. Thought I'd like to thank you for your patience and hopefully I haven't annoyed you and we'll see some good quality patches in the feature from you. :-) Cheers Rico I agree with Rico. It's hard to find issues in your own code, which makes it all the more important to check it thoroughly. Still, at least try to be careful with the code style: +static void RotateX(FLOAT *out, UINT order, FLOAT a, FLOAT *in) I think it's better to only use lower case for the (private) function name. Something like rotatex or rotate_x or whatever. +if ( order 2 ) +return; Please indent it correctly. +return; +} Unneeded. +FLOAT* WINAPI D3DXSHRotate(FLOAT *out, UINT order, CONST D3DXMATRIX *matrix, CONST FLOAT *in) Fix the FLOAT*. +return; +} Again. Also, the patch is full of trailing whitespaces, please fix those too.
Re: d3dx9_36 [try 3]: Implement D3DXSHRotate
On 04.09.2012 19:31, Nozomi Kodama wrote: Your mail makes me thing two or three remarks. I agree that in the plain wine, code must be optimized to use less memory as possible and to be the fastest as possible. But, the tests are here to check that the implemented function behaves like the windows one. So, they don't need to be optimized. So, what is the point to worry about efficiency? It is not important that I allocate 100 floats for my array even if I use 49 only. In the same idea, the time in the loop is not important. It is the same if I spend 1s in a loop or 1/100s. They are there for testing. Tests work, it is OK. I think function is optimized in plain wine, so I can send the patch. Tests don't work. I must figure out why. While I agree with you, partly, one problem doesn't make much trouble, but if you add a couple of problems the app might explode. See the bad example (using a bigger index) for accessing the array... it may work for your machine, but the app may crash on another. You may overwrite some stuff and it is very hard to debug. For the floats, I think it shows that you are familiar with the code and know what you are doing. The question is, why don't you use 100 floats then, shouldn't be a problem at all? And you'll never know the next guy tries to implement something and copies your test code (that's not recommended at all) or just gets an idea and does the same and voila we got more and more of the hassle. Just think about it... I guess you took a look at test_D3DXSHRotateZ which is also a very bad example, but I missed it somehow when it was added. And still no one fixed it yet and that's how bad code and code style comes to that file... About coding style, I can not understand the policy. If I look in the d3dx9_36/math.c file, I can see almost as much styles as functions. So, complaining about the style I use is a non-sense for me. Sure that's the problem. d3dx9_36 (especially math.c) is a very bad example when it comes to code style. Do we have to make it worse? Why do we need to produce more bad examples, if it is relatively easy to write clean code? Cheers Rico
