From owner-svn-src-head@FreeBSD.ORG Tue Feb 3 15:38:50 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 10AE5880; Tue, 3 Feb 2015 15:38:50 +0000 (UTC) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id C76F5EA1; Tue, 3 Feb 2015 15:38:49 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 1B1B73C2A65; Wed, 4 Feb 2015 02:38:41 +1100 (AEDT) Date: Wed, 4 Feb 2015 02:38:40 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" Subject: Re: svn commit: r278154 - head/lib/msun/src In-Reply-To: <201502031417.t13EHU9g074687@svn.freebsd.org> Message-ID: <20150204012614.T1972@besplex.bde.org> References: <201502031417.t13EHU9g074687@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=A5NVYcmG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=mchCe6mdQo55RqhEC7IA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Feb 2015 15:38:50 -0000 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