Code review comment for lp:~epics-core/epics-base/fix-calc-bit-manipulation

Revision history for this message
Andrew Johnson (anj) wrote :

Ok, so I wasn't testing against 3.14 on 32-bit, my bad — the tests do indeed fail there, thanks for pointing that out.

I have committed my changes to the double value tests that I posted yesterday, they are designed to check each cast in the bit-wise operator implementation individually for not overflowing with either +ve or -ve double values that have bit 31 set.

I agree that my caget technique introduced another conversion, I was trying to allow for that in my understanding of what's actually going on. Your testUInt32Calc() routine does the same conversion though:
    uresult = (epicsUInt32) result;
We could avoid that completely by doing the test comparisons inside the expression being tested:
- testUInt32Calc("0xaaaaaaaa AND 0xffff0000", 0xaaaa0000u);
+ testCalc("(0xaaaaaaaa AND 0xffff0000) == 0xaaaa0000", 1);
However the returned result is less useful for diagnosing overflow problems, so I'm happy to keep what we have now.

If you're happy with this I will merge it.

review: Approve

« Back to merge proposal