uniqnum: Update and remove undefined C behavior - #143
Conversation
Since 5.6, Configure calculates if all possible UV/IV's values can be contained losslessly in an NV. Fallback to our previous determination for earlier perls.
Use the value calculated by Configure, falling back to 53 if not available. It became available during 5.7 development
It is a good assumption; but not a fully accurate one, and it's trivial to fix.
There were several undefined behaviors marked by running ASan on this code; all stemmed from using signed values, where unsigned would work. There was an issue with abs(IV_MIN) not being representable in an IV on a 2's complement machine; and left shifting a negative value is undefined. This commit solves this by converting to use an unsigned value, and stealing the macro now in the perl core to take the abs(IV_MIN) without undefined behavior. I found myself drawing a picture to try to understand the behavior of this. And I think the picture is clearer than the previous text. so I changed to use it. And the intermediate results that were there really just interrupted the flow of the picture, so I removed them.
NB: Starting with C23, C requires two's complement. |
|
This PR builds and tests fine for me on MS Windows, on a wide variety of perls across versions 5.16.3 to 5.43.1 - including 32-bit, 64-bit, -Duselongdouble, -Dusequadmath. One thing I noticed was that t/uniqnum.t produced the following warning (but only in the 5.42.0 and 5.43.1 builds) : I don't think that's even related to the changes made by this PR, but line 215 is: I was able to silence the warning by changing that line to I think that retains the original (and intended) behaviour. |
|
Gosh; that original code is kinda crazy - it really needs a first-line "hey here's what we're doing" to explain what is going on... but anyhow... Yes I think this all looks fine, but the 5.6 check fails on it. The logs have now been deleted so we can't see why. Is there a way to rerun this? |
uniqnumassumes this is a 2's complement machine. Change to not assume that.