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

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

Added an epicsUInt32 utop variable to calcPerform() to reduce the number of casts, and added some code comments.

Previous versions have always handled shifts as signed quantities, so the sign bit is extended on right-shifts. I changed the left-shift code to match, although it doesn't actually make any difference.

The shift operators also need to have their shift count limited to avoid Undefined Behavior; I see 2 realistic possibilities: ANDing the shift count with 31; or return 0 on shift-overflow but when right-shifting a negative quantity return -1 on shift-overflow. The first is the fastest and simplest to code, but the second one might be regarded as be more logical. Any preferences or alternative suggestions? I have implemented the first option for now.

The large positive decimal values in your tests are causing the same overflows when converted from double to integer that Dirk originally reported, they always convert to 0x80000000 on my RHEL systems so some tests fail. The paragraph before Dirk's "The fix is:" line explains that you have to use negative values for bit 31 to be set, which my tests agree with. The negative versions that work are 0xaaaaaaaa = -1431655766, 0xffff0000 = -65536. Note that postfix() compiles double literals into a LITERAL_INT op-code if the value can be safely held in an epicsInt32, so adding ".0" has no effect; but using ".1" instead does work, the fraction gets truncated.

Also fixed some minor white-space issues.

review: Approve

« Back to merge proposal