RE: [PATCH] Fix undefined behaviour in lround,llround.

["Howland Craig D (Craig)" <howland at LGSInnovations dot com>]

On 16/07/2010 14:15, Dave Korn wrote:
>  I make it that the "SAFE_RIGHT_SHIFT (tmp, (52 -
exponent_less_1023))" is
>necessary because 52 - 20 == 32.
>
>  It was the SAFE_LEFT_SHIFT of that pair that I thought was
unnecessary, and
>the SAFE_RIGHT_SHIFT of tmp above, both of which have amt ==
>(exponent_less_1023 - 20), but I left it in for consistency and hoped
the
>compiler would be clever enough to get rid of it.  I'd be happy to omit
those
>two if that's how folks prefer the consistency/efficiency trade-off.

     I thought of that right after I hit "send":  yes, really only
the left-most shift (exp-20) needs the safe shift.  (At least, as long
as
long long is not larger than 64.  But in that (future) case, the code
will
be broken for a different reason, too.)
     I'd propose that the shifts be put in only where they are
absolutely
needed, because then it is clear.  If un-needed ones are present, people
later on will be wondering why they are they, as to what they are
missing
(which would be nothing).  So I think that just one SAFE_SHIFT per file
is needed and appropriate.

>
>> I think that this error is likely in more files (lrint is likely),
>> as there are many similarites.  (I'll have to take a look later.)
>
>  Thanks.  I convinced myself that the rounding functions that use the
TWO52[]
>array are safe, but a second pair of eyeballs would certainly be good.

     I agree that the TWO52[] stuff, itself, is safe, but lrint() and
llrint() have, on a quick glance, the exact same problem shift as
lround()
does for the >= 52 case.

Craig