#15864 - Miked0801 - Sun Feb 01, 2004 10:00 pm
As promised, here's a little snippet that has issues. Difficulty on this one ranks about 4 on a scale from 1 to 10.
| Code: |
#define FIXED_TO_INT(x) x >> 16
s32 mathFixedMulToInt(s32 fixed1, s32 fixed2)
{
return (FIXED_TO_INT(fixed1) * FIXED_TO_INT(fixed2));
}
|
A couple things not right, some harder to spot that others. Enjoy!
#15867 - poslundc - Sun Feb 01, 2004 11:20 pm
I'm bored so I'll play.
- No parentheses around the FIXED_TO_INT macro. So the formula is (x >> 16 * x >> 16) which is almost certainly not what is intended. (-Wall will generate a warning for this in gcc.)
- No parentheses around the x in the FIXED_TO_INT macro, which is liable to cause more trouble in other situations (although not this particular function) where the macro is used on expressions instead of single variables/values.
- The function assumes 16.16 point notation. (At least it probably does.) It might not be. Using a more informative typedef would help (eg. typedef s32 Fixed16x16).
- If the function did work, it would be eliminating all precision before multiplying. A more sound strategy for a general-purpose 16.16 multiplication would be to reduce both parameters to 24.8 before multiplying so you would obtain another 16.16 result. (You can then reduce down to 32.0 if you want to conform with the mathFixedMulToInt spec.)
Finally - and this is not a correctness aspect - I would make this an inline function or put the entire thing in a macro rather than forcing a context switch just for a simple multiply routine.
That's all I caught... let me know if I missed anything.
Dan.
#15868 - jd - Sun Feb 01, 2004 11:35 pm
This would expand to:
| Code: |
s32 mathFixedMulToInt(s32 fixed1, s32 fixed2)
{
return (fixed1 >> 16 * fixed2 >> 16);
}
|
I can't remember my operand priorities (I usually use lots of brackets to make sure) but I think that "*" has a higher precidence than ">>". This would mean that the code above would be equivalent to:
| Code: |
s32 mathFixedMulToInt(s32 fixed1, s32 fixed2)
{
return (fixed1 >> (16 * fixed2) >> 16);
}
|
This is almost certainly not what the original programmer intended. The only other thing that I can think of is that s32 isn't defined in the code so that could be declared incorrectly as well - although perhaps I'm being a bit too pedantic there.
Edit: Darn, poslundc beat me to it. :)
#15869 - Miked0801 - Sun Feb 01, 2004 11:41 pm
Lol. That didn't take too long. I expected people to see the missing parenthesis and not notice I was throwing away all the precision on the multiply. BTW, if I was doing this for real, I'd have a 'fixed' typedef sitting around so that notation would make more sense. A -O3 would more than likely inline this :)
I've got a couple more fun ones that aren't quite as straight forward I'll post during the superbowl if the comercial suck - otherwise I'll throw them up tomorrow.
Good fun!
#15872 - dagamer34 - Mon Feb 02, 2004 4:19 am
Again, Waldo(s)....
_________________
Little kids and Playstation 2's don't mix. :(