Date: Wed, 4 Feb 2015 03:51:55 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r278154 - head/lib/msun/src Message-ID: <20150204031811.N2584@besplex.bde.org> In-Reply-To: <54D0F29C.2090401@FreeBSD.org> References: <201502031417.t13EHU9g074687@svn.freebsd.org> <20150204012614.T1972@besplex.bde.org> <54D0F29C.2090401@FreeBSD.org>
index | next in thread | previous in thread | raw e-mail
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
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150204031811.N2584>