Re: CURRENT: CLANG 3.3 and -stad=c++11 and -stdlib=libc++: isnan()/isninf() oddity

On Thu, 11 Jul 2013, David Chisnall wrote:

> On 11 Jul 2013, at 13:11, Bruce Evans <brde_at_optusnet.com.au> wrote:
>
>> The error message for the __builtin_isnan() version is slightly better up
>> to where it says more.
>>
>> The less-unportable macro can do more classification and detect problems
>> at compile time using __typeof().
>
> The attached patch fixes the related test cases in the libc++ test suite.  Please review.

OK if the ifdefs work and the style bugs are fixed.

> This does not use __builtin_isnan(), but it does:
>
> - Stop exposing isnan and isinf in the header.  We already have __isinf in libc, so this is used instead.
>
> - Call the static functions for isnan __inline__isnan*() so that they don't conflict with the ones in libm.
>
> - Add an __fp_type_select() macro that uses either __Generic(), __builtin_choose_expr() / __builtin_choose_expr(), or sizeof() comparisons, depending on what the compiler supports.
>
> - Refactor all of the type-generic macros to use __fp_type_select().

% Index: src/math.h
% ===================================================================
% --- src/math.h	(revision 253148)
% +++ src/math.h	(working copy)
% _at__at_ -80,28 +80,39 _at__at_
%  #define	FP_NORMAL	0x04
%  #define	FP_SUBNORMAL	0x08
%  #define	FP_ZERO		0x10
% +
% +#if __STDC_VERSION__ >= 201112L
% +#define __fp_type_select(x, f, d, ld) _Generic((x),     \
% +	float: f(x),                                    \
% +	double: d(x),                                   \
% +	long double: ld(x))

The normal formatting of this is unclear.  Except for the tab after #define.
math.h has only 1 other instance of a space after #define.

% +#elif __GNUC_PREREQ__(5, 1)
% +#define __fp_type_select(x, f, d, ld) __builtin_choose_expr(                   \
% +	__builtin_types_compatible_p(__typeof (x), long double), ld(x),        \
% +	 __builtin_choose_expr(                                                \
% +	  __builtin_types_compatible_p(__typeof (x), double), d(x),            \
% +	   __builtin_choose_expr(                                              \
% +	    __builtin_types_compatible_p(__typeof (x), float), f(x), (void)0)))

Extra space after __typeof.

Normal formatting doesn't march to the right like this...

% +#else
% +#define __fp_type_select(x, f, d, ld)                         \
% +	((sizeof (x) == sizeof (float)) ? f(x)                \
% +	     : (sizeof (x) == sizeof (double)) ? d(x)         \
% +	     : ld(x))

... or like this.

Extra space after sizeof (bug copied from old code).

% +#endif
% +
% +
% +

Extra blank lines.

%  #define	fpclassify(x) \
% -    ((sizeof (x) == sizeof (float)) ? __fpclassifyf(x) \
% -    : (sizeof (x) == sizeof (double)) ? __fpclassifyd(x) \
% -    : __fpclassifyl(x))

Example of normal style in old code (except for the space after sizeof(),
and the backslashes aren't line up like they are in some other places in
this file).

% ...
% _at__at_ -119,10 +130,8 _at__at_
%  #define	isunordered(x, y)	(isnan(x) || isnan(y))
%  #endif /* __MATH_BUILTIN_RELOPS */
% 
% -#define	signbit(x)					\
% -    ((sizeof (x) == sizeof (float)) ? __signbitf(x)	\
% -    : (sizeof (x) == sizeof (double)) ? __signbit(x)	\
% -    : __signbitl(x))
% +#define signbit(x) \
% +	__fp_type_select(x, __signbitf, __signbit, __signbitl)

The tab lossage is especially obvious here.

This macro definition fits on 1 line now.  Similarly for others except
__inline_isnan*, which takes 2 lines.  __inline_isnan* should be named
less verbosely, without __inline.  I think this doesn't cause any
significant conflicts with libm.  Might need __always_inline.
__fp_type_select is also verbose.

% 
%  typedef	__double_t	double_t;
%  typedef	__float_t	float_t;
% _at__at_ -175,6 +184,7 _at__at_
%  int	__isfinite(double) __pure2;
%  int	__isfinitel(long double) __pure2;
%  int	__isinff(float) __pure2;
% +int	__isinf(double) __pure2;
%  int	__isinfl(long double) __pure2;
%  int	__isnanf(float) __pure2;
%  int	__isnanl(long double) __pure2;
% _at__at_ -185,6 +195,23 _at__at_
%  int	__signbitf(float) __pure2;
%  int	__signbitl(long double) __pure2;

The declarations of old extern functions can probably be removed too
when they are replaced by inlines (only __isnan*() for now) .  I think
the declarations of __isnan*() are now only used to prevent warnings
(at higher warning levels than have ever been used) in the file that
implement the functions.

% 
% +static __inline int
% +__inline_isnanf(float __x)
% +{
% +	return (__x != __x);
% +}
% +static __inline int
% +__inline_isnan(double __x)
% +{
% +	return (__x != __x);
% +}
% +static __inline int
% +__inline_isnanl(long double __x)
% +{
% +	return (__x != __x);
% +}
% +
% +

Extra blank lines.

Some insertion sort errors.  In this file, APIs are mostly sorted in the
order double, float, long double.

All the inline functions except __inline_isnan*() only evaluate their
args once, so they can be simpler shorter and less namespace-polluting
as macros.  catrig*.c uses the following macros for them (just showing
the double precision ones here):

_at_ #define isinf(x)	(fabs(x) == INFINITY)
_at_ #define isnan(x)	((x) != (x))
_at_ #define signbit(x)	(__builtin_signbit(x))

Note that that these are not quite independent of the precision, so
they don't all need multiple inline functions or macros.  fabs() is
not type-generic, but __builtin_fabs() might be.  fabsl() for all
precisions would work but be slower.  __builtin_signbit() is even more
likely to be type-generic, like __builtin_isnan().  But catrig[fl].c
spell out the type suffixes for safety.

I hoped to copy these to math.h after fixing any technical problems.
isnan() needs a statement-expression to be safe.  signbit() is more
of the problem than the others, since it is hard to do without a builtin
and the builtin doesn't exist in gcc-3.x.  The builtin works fine with
-current gcc and clang, at least on x86 .  So the committed version
just uses the builtin, and my local version uses libm internals that
are unsuitable for the public signbit() (mainly due to namespace
problems).  The builtin might not work right in -curent for non-x86.
When the extern function is used, it uses similar libm internals, but
they take much longer when not inlined.

Bruce