Re: d3dx9_36 [try 3]: Implement D3DXSHRotate

[Rico Schüller]

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

d3dx9_36 [try 3]: Implement D3DXSHRotate

[Matteo Bruni]

2012/9/4 Rico Schüller :
> 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.

d3dx9_36 [try 3]: Implement D3DXSHRotate

[Nozomi Kodama]

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

d3dx9_36 [try 3]: Implement D3DXSHRotate

[Nozomi Kodama]

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 
À : [e-mail]; Nozomi Kodama  
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

d3dx9_36 [try 3]: Implement D3DXSHRotate

[Rico Schüller]

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 1000000 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