Re: Fw: [PATCH 3/6] Avoid implicit floating point conversions

Keith Packard via Newlib <[email protected]> Thu, 20 Aug 2020 17:01:53 -0700
Newsgroups	gmane.comp.lib.newlib
Message-ID	<[email protected]>
C Howland via Newlib <[email protected]> writes:

> There are many troubles here along with many good things.  Some specific
> ones are noted below, but a general one is that clang appears to be
> generating spurious complaints--or else we need to fix something else
> related to using clang.  The general problem is with replacing isnan() and
> isinf() with isnanf() and isinff().  This is spurious because as of C99
> isnan() and isinf() are defined to be macros that take a real-floating
> type; they should be fine as they are.  In addition, the f variants of
> those are not standard and ought to be called as __isinff() and __isnan()
> if the subs really still need to be done.  Doesn't clang have something
> like GCC's __builtin_isnan() to be used, for example?

Right, but the newlib header files weren't using it due to the tests
being GCC specific. I've fixed that and the resulting patch is
significantly smaller. I'll be posting that as a short series
separately.

> The standard defines _Complex_I as being type const float _Complex.

Thanks; that really helps my understanding here. I've removed those
changes and added a (bunch of) casts from these constants to the
appropriate type when used inside the library.

>>  float
>>  _redupif(float x)
>> @@ -77,7 +77,7 @@ _redupif(float x)
>>
>>         i = t;  /* the multiple */
>>         t = i;
>> -       t = ((x - t * DP1) - t * DP2) - t * DP3;
>> +       t = (((double) x - (double) t * DP1) - (double) t * DP2) -
>> (double) t * DP3;
>>
> This is simply nuts:  the language clearly defines promotion rules.  All
> get turned into double if there's a single double in it, so one cast should
> be enough. If the compiler complains otherwise we should be complaining
> about the compiler, not kowtowing to its warnings.  We should not allow
> compiler warnings to make code unreadable.

Yes, the default promotion rules work for this case, but enabling use of
this extra clang warning resulted in finding numerous places where the
default promotion rules were causing computations to be done in higher
precision than necessary. And, enabling the warning encourages "fixing"
even cases where the code is correct.

So, we can either leave the code generating warnings with the clang
flag, use this (rather ugly) version, or introduce some temporary locals
of 'double' type and eliminate the casts from this line.

> I did not exhaustively look at every change, only scanning a bit towards
> the end.  Except as noted, seemed generally sane.

Thanks for your review; all of these fixes have been run through the
newlib test suite on a rather large number of architectures without
introducing any changes.

> I see that Joseph Myers types faster than me and so some of these comments
> have already  been made, but since I've already typed it in I'm not going
> back to re-edit.

I'll make sure he hasn't caught anything additional; thanks to both of
you for fixing my mis-understanding of the types of the various math.h
constants!

New series coming shortly.

-- 
-keith
signature.asc (application/pgp-signature, 832 B)