mfinzi / libfixmath

Automatically exported from code.google.com/p/libfixmath
0 stars 0 forks source link

CPP interface misleading #8

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Doing the following will cause unexpected results;

Fix16 test = 5.0;
int divider = 2;
test /= divider;

The expected result would be 5.0, however it will instead be -32768.0 or 
similar. This is because in C++ fix16_t is treated as an int, so that the 
interface assumes the value 2 to be a fixed point value and not an integer.

The following code will work:
Fix16 test = 5.0;
int16_t divider = 2;
test /= divider;

This will produce the value 2.5 as expected because the interface treats 
int16_t as an integer.

The problem is that I can't see a way to have the fix16_t type interoperable 
with Fix16 class without sacrificing support for readable int maths. I'm not 
sure that it's sensible to leave it working this way either though.

Original issue reported on code.google.com by Flatmush@googlemail.com on 2 Mar 2011 at 1:17

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Hi,

As far as I can tell, the problem also occurs in the Fix16(const fix16_t) 
constructor. So Fix16 myInteger(42); yields a fraction instead of a whole 
number. There is a solution, but it breaks the semantics of the existing 
interface.

- Add a signed and unsigned integer constructor that has whole number semantics 
(i.e. treats the value as an integer).
- Remove the Fix16(const fix16_t) constructor, or make it protected and add a 
dummy boolean second argument. Maybe making the constructor explicit has the 
same effect. That prevents unwanted automatic casts. It should then only be 
used to define internal constants, although this does not appear to be 
necessary in the current implementation. So removing the constructor altogether 
appears preferable.
- Remove all +, -, *, /, +=, -=, *=, and /= operators that do not take a Fix16 
argument as rvalue. The compiler will use the constructors to cast/transform 
the rvalue, so everything still works. The same applies to the sadd, ssub, smul 
and sdiv operations.
- Remove operator fix16_t(). It is dangerous when assigning a Fix16 to an 
integer.
- Ideally make the class attribute 'value' protected. Provide a raw() operation 
that returns the value if needed.

So yes, these changes affect the existing interface, but that is not 
necessarily a bad thing. The current interface leaves a thing or two to be 
desired in terms of preventing honest mistakes on the part of the user. 
Personally I value preventing such mistakes over maintaining the current 
interface.

Original comment by AlexVanD...@gmail.com on 25 Jul 2011 at 5:12