search

kobalicek / mathpresso

Mathematical Expression Parser and JIT Compiler
Other
141 stars 16 forks source link

small bug with inbuilt function isinf #14

Closed Hades0299 closed 7 years ago

Hades0299 commented 7 years ago

While working with the code, i noticed, that the assemblerimplementation of isinf gives a wrong result. In mpCompiler in Line 380 the operand gets compared with the constant 0xFF80000000000000. Correct, as of IEEE754, would be 0xFFF0000000000000, as the exponent is 11 bit (with one signbit)(src).

Regards Hades

kobalicek commented 7 years ago

I will check it out, shouldn't it be 0x7FF0000000000000 to ignore the sign then? But I haven't looked yet, let me check it out first

kobalicek commented 7 years ago

Hmm Ok now I can see that, it should really be 0xFFF0000000000000, let me do few tests first

Hades0299 commented 7 years ago

I noticed, that the function mpGetNan (defined in mpeval_p.h) returns 0x7FF8000000000000 as well, here 0x7FFFFFFFFFFFFFFF would be correct. Also mpGetInf returns 0x7FF0000000000000, and not 0x7FFF000000000000, which would be positive infinity. The same goes for the methods in the union DoubleBits.

Is there a reason, why you don't use the the functions std::isNan(), std::isInf(), std::numerical_limits::quiet_NaN() or std::numeric_limits::infinity()?

Regards Hades

kobalicek commented 7 years ago

I committed some changes that should fix the INF issue and add more tests, it's now running on CI.

I think that NAN is not that easy, there is many NANs and basically any pattern 0x7FFxxxxxxxxxxxxx would work if xxxxxxxxxxxxx != 0, as far as I know Intel|AMD produces 0xFFF8000000000000 (quiet nan), so I choose numbers that are consistent.

I think not using std was mostly historical, I wanted more compatibility initially. Now after asmjit already uses some std things I think it's time to reconsider that.

Hades0299 commented 7 years ago

Thanks for the changes. With the NAN you are right. I thought, that setting all bits to 1 would be easier to distinguish from non-NAN-values. And it looks like i should doublecheck what I write, mpGetInfwas right all along, somehow I added a third F to the HEX-value. I Guess this could be closed now, if you do not want to track your decision regarding switching to std-functions here.

Regards Hades

kobalicek commented 7 years ago

Yeah I would probably close this and open another one if necessary.