[PATCH] Provide a C++ version of iseqsig

[Jonathan Wakely]

On 03/11/17 11:16 -0200, Gabriel F. T. Gomes wrote:
>I would like to receive some feedback on the correctness of this
>implementation, more specifically on the correctness of any implicit
>type conversions, which I might have missed.  I'm working on a test case
>in the meantime (I only did some standalone tests outside of glibc test
>suite).
>
>-- 8< --
>In C++ mode, __MATH_TG cannot be used for defining iseqsig, because
>__MATH_TG relies on __builtin_types_compatible_p, which is a C-only
>builtin.  This is true when float128 is provided as an ABI-distinct type
>from long double.
>
>Moreover, the comparison macros from ISO C take two floating-point
>arguments, which need not have the same type.  Choosing what underlying
>function to call requires evaluating the formats of the arguments, then
>selecting which is wider.  The macro __MATH_EVAL_FMT2 provides this
>information, however, only the type of the macro expansion is relevant
>(actually evaluating the expression would be incorrect).
>
>This patch provides a C++ version of iseqsig, in which only the type
>resulted from a call to __MATH_EVAL_FMT2 is used as an additional
>argument, fmt, to the helper function, __iseqsig_type.  This function
>is overloaded, in compilation-time, to the floating-point type specified
>by the fmt argument, then calls the appropriate underlying function (the
>type of the arguments is left unchanged with the help of templates).
>
>Tested for powerpc64le and x86_64.
>
>	[BZ #22377]
>	* math/math.h [C++] (iseqsig): New implementation, which does
>	not rely on __MATH_TG/__builtin_types_compatible_p.
>---
> math/math.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
>diff --git a/math/math.h b/math/math.h
>index 326fd8ebe1..4744f9a4c9 100644
>--- a/math/math.h
>+++ b/math/math.h
>@@ -1152,8 +1152,56 @@ iszero (__T __val)
>
> /* Return X == Y but raising "invalid" and setting errno if X or Y is
>    a NaN.  */
>+# ifndef __cplusplus
>+#  define iseqsig(x, y) \
>+   __MATH_TG (__MATH_EVAL_FMT2 (x, y), __iseqsig, ((x), (y)))
>+# else
>+/* In C++ mode, __MATH_TG cannot be used, because it relies on
>+   __builtin_types_compatible_p, which is a C-only builtin.  Moreover,
>+   the comparison macros from ISO C take two floating-point arguments,
>+   which need not have the same type.  Choosing what underlying function
>+   to call requires evaluating the formats of the arguments, then
>+   selecting which is wider.  The macro __MATH_EVAL_FMT2 provides this
>+   information, however, only the type of the macro expansion is
>+   relevant (actually evaluating the expression would be incorrect).
>+   Thus, the type is used in an additional argument, fmt, to the helper
>+   function, __iseqsig_type, which is overloaded in compilation-time for
>+   the correct floating-point type, then calls the appropriate
>+   underlying function (the type of the arguments is unchanged with the
>+   help of templates).  */
>+extern "C++" {
>+template <typename __T1, typename __T2> inline int

I don't know what the glibc convention is, but in libstdc++ we'd just
use _T1 here, not __T1. The double-underscore is not needed when the
first letter is uppercase.

>+__iseqsig_type (float fmt, __T1 x, __T2 y)

All the "fmt" and "x" and "y" parameters need to use reserved names,
i.e. __fmt, __x and __y.

But I wouldn't bother naming the __fmt parameter at all. It's unused,
so will produce warnings with -Wsystem-headers. C++ allows unused
parameters to be unnamed, so just say:

__iseqsig_type (float, __T1 __x, __T2 __y)

>+{
>+  return __iseqsigf (x, y);
>+}
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (double fmt, __T1 x, __T2 y)
>+{
>+  return __iseqsig (x, y);
>+}
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (long double fmt, __T1 x, __T2 y)
>+{
>+#  ifdef __NO_LONG_DOUBLE_MATH
>+  return __iseqsig (x, y);
>+#  else
>+  return __iseqsigl (x, y);
>+#  endif
>+}
>+#  if __HAVE_DISTINCT_FLOAT128
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (_Float128 fmt, __T1 x, __T2 y)
>+{
>+  return __iseqsigf128 (x, y);
>+}
>+#  endif
>+}
>+# endif
>+
> # define iseqsig(x, y) \
>-  __MATH_TG (__MATH_EVAL_FMT2 (x, y), __iseqsig, ((x), (y)))
>+  __iseqsig_type (__MATH_EVAL_FMT2 (x, y), x, y)

Why define this as a macro, not an inline function template?

template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y)
  {
    return __iseqsig_type (__MATH_EVAL_FMT2 (__x, __y), __x, __y);
  }

The C++ standard explicitly requires all functions from the C library
to be defined as real functions, and *not* macros. This means the C++
library has to do #undef for every function from the C library that
might be defined as a macro. Doing that here would make iseqsig
unusable, so a C++ implementation that wanted to define it would need
to provide its own definition. If you define an inline function it
just works, and there's no problem. Most C++ programmers want fewer
macros, not more.

Also, should it be declared to not throw exceptions?

Apart from those points, your solution will work, but it results in
two different instantiations of the function template for calls to
iseqsig(1.0, 1.0f) and iseqsig(1.0f, 1.0).

The first one will call:

  __iseqsig_type<double, float>(double, double, float);

and the second will call:

  __iseqsig_type<float, double>(double, float, double);

If they are inlined then it won't matter, but if not you'll generate
twice as much code as needed (and also more debug info).

If you're allowed to use GCC's __typeof__ (or __decltype__) then I'd
do it like this:

extern "C++" {
template<typename> struct __iseqsig_type;

template<> struct __iseqsig_type<float> {
  static int __call(float __x, float __y) throw()
  { return __iseqsigf (__x, __y); }
};

template<> struct __iseqsig_type<double> {
  static int __call(double __x, double __y) throw()
  { return __iseqsig (__x, __y); }
};

// ... long double and float128 specializations ...

template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y) throw()
  {
    typedef __typeof__ (__MATH_EVAL_FMT2 (__x, __y)) _T3;
    return __iseqsig_type<_T3>::__call(__x, __y);
  }
}

If you can't use __typeof__, you could do what libstdc++ does for
<cmath> overloads: provide non-template functions for the cases where
the parameters are the same:

extern "C++" {
inline int iseqsig(float __x, float __y) throw() { return __iseqsigf(__x, __y); }
inline int iseqsig(double __x, double __y) throw() { return __iseqsig(__x, __y); }
// ... long double and float128 overloads ...

And then provide a function template to handle mixed types, which
converts both arguments to the promoted type:

// Handle mixed argument types.
template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y) throw()
  {
    return iseqsig(true ? __x : __MATH_EVAL_FMT2 (__x, __y),
                   true ? __y : __MATH_EVAL_FMT2 (__x, __y));
  }
}

The conditional expression will always return the first operand, but
will promote it to the type of the second if needed.