Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 4 Feb 2015 02:38:40 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        "Pedro F. Giffuni" <pfg@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r278154 - head/lib/msun/src
Message-ID:  <20150204012614.T1972@besplex.bde.org>
In-Reply-To: <201502031417.t13EHU9g074687@svn.freebsd.org>
References:  <201502031417.t13EHU9g074687@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 3 Feb 2015, Pedro F. Giffuni wrote:

> Log:
>  Reduce confusion in scalbnl() family of functions.
>
>  The changes unrelated to the bug in r277948 made
>  the code very difficult to understand to both
>  coverity and regular humans so take a step back
>  to something that is much easier to understand
>  for both and follows better the original code.
> 
>  CID:	1267992, 1267993, 1267994
>  Discussed with:	kargl

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

You also added style bugs (excessive parentheses) in only 1 of 3 places.

> 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.

> +	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.

The second point that is hard to understand for this cast is that its
behaviour is only implementation-defined.  Thus it delivers a result
and has no bad side effects.

The third point that is hard to understand about this cast is that its
result doesn't matter.  The usual result is the original n truncated,
but it can be complete garbage.  In both cases, truncation is detected
by (in != n), and we proceed to adjust the original n in the truncated
cases.  Implementation-defined behaviour is usually much harder to
understand than this.  Some entity has to read the system documentation
for all supported systems (and sometimes first file bug reports to get
it written) and write ifdefs for all cases.  This is hard for compilers
to do.  Here they only have to read the C standard to check that the
bahaviour is not undefined.  Even language lawyers might need to check
this.

The fourth point that is (not so) hard to understand about this cast is
that after detecting truncation, we don't use the truncated n again to
complete the adjustment.

> +	if (in != n)
> +		in = (n > 0) ? INT_MAX: INT_MIN;

Instead of an idiomatic conditional ladder, this now uses a combination of
an if clause and simpler conditional clause.  Before this, many
complications are introduced by the implementation-defined behaviour.
We use the else clause to separate the truncated case from the truncated
case.  Then in the truncated case, we still use a conditional clause
which non-C programmers might find confusing, and it is essentially the
first part of the conditional ladder written in an obfuscated way.
Compare:

first part of ladder:
 	(n > INT_MAX) ? INT_MAX : (n < INT_MIN) ? INT_MIN
replacement:
 		(n > 0) ? INT_MAX: INT_MIN

(n > 0) is an obfuscated/manually microoptimized way of writing
(n > INT_MAX).  It means the same since we are in the truncated case,
so n cannot be between 0 and INT_MAX.  The negation of (n > 0) means
(n < INT_MIN), similarly.

As pointed out in my review, the range checks could be further unobfuscated
to check the limits on the exponents instead of INT_MIN and INT_MAX.
These limits are significantly smaller.  Using them, the magic cast
to int just gets in the way.  Magic still occurs in the final conversion,
and it is best to leave the final n in my version uncast so that the
compiler can detect problems in the algorithm or implementation.  The
magic is that the natural limits are smaller than INT_MIN and INT_MAX;
this if we restrict to them we automatically restrict to values that can
be represented as an int.  Then if we make a mistake in this, the compiler
has a chance of warning provided we don't kill the warning using the cast.
We are only using INT_MIN and INT_MAX in the first place since they are
slightly easier to spell than the natural limits (but harder to spell
hard-coded limits that should be enough for any FP format).  We know by
non-magic that larger-than necessary limits give the same results.

> 	return (scalbn(x, in));
> }
>

Similarly in the other functions, except scalblnl() has excessive braces.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150204012614.T1972>