svn commit: r278154 - head/lib/msun/src

[Bruce Evans]

On Tue, 3 Feb 2015, Pedro Giffuni wrote:

> On 03/02/2015 10:38 a.m., Bruce Evans wrote:

Please trim quotes.

[... quotes truncated]

>> You mean, take a step backwards to something that is harder to understand.

>>> Modified: head/lib/msun/src/s_scalbln.c
>>> ============================================================================== 
>>> --- head/lib/msun/src/s_scalbln.c    Tue Feb  3 13:45:06 2015 (r278153)
>>> +++ head/lib/msun/src/s_scalbln.c    Tue Feb  3 14:17:29 2015 (r278154)
>>> @@ -35,7 +35,9 @@ scalbln (double x, long n)
>>> {
>>>     int in;
>>> 
>>> -    in = (n > INT_MAX) ? INT_MAX : (n < INT_MIN) ? INT_MIN : n;
>> 
>> It is a feature that this detected a bug in Coverity.  I intentionally
>> left out the cast of n at the end, after even more intentionally leaving
>> it out at the beginning.  It is obvious to humans^WC programmers, that
>> n has been checked to fit in an int; thus any compiler warning about n
>> possibly being truncated is a compiler defect.  It is especially important
>> that compilers understand explicit range checks and don't warn about
>> conversions that cannot change the value when the range checks have been
>> satisfied.
>
> I noticed the implicit cast, but then the fact that you triggered a bug in 
> coverity,
> could be an indication that you could also trigger the same bug in a compiler
> optimizer so some bogus compiler may end up optimizing the above check into
> /*do nothing */.

Not likely.

>>> +    in = (int)n;
>> 
>> This restores a hard-to understand cast.  Compilers cannot reasonably
>> understand this at all.  Without the cast, they should (optionally)
>> warn about possible truncation.  But the truncation is intentional.
>> The programmer is using the cast to inhibit the warning (the case has no
>> other effect).  A higher level of warnings would still warn about it,
>> and my rewrite was to prevent this and to make the code easier to
>> understand.
>> ...
> I honestly only wanted to clean the original bug but all in all, I find our
> implementation of this functions somewhat disappointing: shouldn't we
> be doing real calculations with the requested precision instead of wrapping
> them around the int counterparts? It looks like NetBSD does implement them.

That would be a silly micro-optimization for functions that are almost
never used.  We are doing real calculations, with a simple translation
for overflowing and undeflowing cases to avoid duplicating code.

Now I wonder why these functions (the ones taking a long arg) even
exist.  According to a Linux man page which seems to be quoting a POSIX
spec, it is to support systems with 16-bit ints and unusually large
LDBL_MAX_EXP.  FreeBSD only supports systems where LDBL_MAX_EXP is at
most 16384, and this fits in a 16-bit int.  Very little code supports
these unusual systems.  Most code uses plain assumes 16-bit ints and/or
usual LDBL_MAX_EXP, and just uses scalbn*().  POSIX now requires 32-bit
ints, so this rationale certainly doesn't apply to it.  It applies to
C99.  So the sloppy code is now portable in POSIX but not in C99.

Bruce