diff mbox series

Add C++ versions of iscanonical for ldbl-96 and ldbl-128ibm

Message ID 20171003140307.22359-1-gabriel@inconstante.eti.br
State New
Headers show
Series Add C++ versions of iscanonical for ldbl-96 and ldbl-128ibm | expand

Commit Message

Gabriel F. T. Gomes Oct. 3, 2017, 2:03 p.m. UTC 
All representations of floating-point numbers in types with IEC 60559
binary exchange format are canonical.  On the other hand, types with IEC
60559 extended formats, such as those implemented under ldbl-96 and
ldbl-128ibm, contain representations that are not canonical.

TS 18661-1 introduced the type-generic macro iscanonical, which returns
whether a floating-point value is canonical or not.  In Glibc, this
type-generic macro is implemented using the macro __MATH_TG, which, when
support for float128 is enabled, relies on __builtin_types_compatible_p
to select between floating-point types.  However, this use of
iscanonical breaks C++ applications, because the builtin is only
available in C mode.

This patch provides a C++ implementation of iscanonical that relies on
function overloading, rather than builtins, to select between
floating-point types.

Unlike the C++ implementations for iszero and issignaling, this
implementation ignores __NO_LONG_DOUBLE_MATH.  The double type always
matches IEC 60559 double format, which is always canonical.  Thus, when
double and long double are the same (__NO_LONG_DOUBLE_MATH), iscanonical
always returns 1 and is not implemented with __MATH_TG.

Tested for powerpc64, powerpc64le and x86_64.

	[BZ #22235]
	* math/Makefile [CXX] (tests): Add test-math-iscanonical.cc.
	(CFLAGS-test-math-iscanonical.cc): New variable.
	* math/test-math-iscanonical.cc: New file.
	* sysdeps/ieee754/ldbl-96/bits/iscanonical.h (iscanonical):
	Provide a C++ implementation based on function overloading,
	rather than using __MATH_TG, which uses C-only builtins.
	* sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h (iscanonical):
	Likewise.
	* sysdeps/powerpc/powerpc64le/Makefile
	(CFLAGS-test-math-iscanonical.cc): New variable.
---
 math/Makefile                                  |  4 ++-
 math/test-math-iscanonical.cc                  | 48 ++++++++++++++++++++++++++
 sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h | 21 +++++++++--
 sysdeps/ieee754/ldbl-96/bits/iscanonical.h     | 19 +++++++++-
 sysdeps/powerpc/powerpc64le/Makefile           |  1 +
 5 files changed, 89 insertions(+), 4 deletions(-)
 create mode 100644 math/test-math-iscanonical.cc

Comments

Joseph Myers Oct. 3, 2017, 2:54 p.m. UTC | #1 
On Tue, 3 Oct 2017, Gabriel F. T. Gomes wrote:

> 	[BZ #22235]
> 	* math/Makefile [CXX] (tests): Add test-math-iscanonical.cc.
> 	(CFLAGS-test-math-iscanonical.cc): New variable.
> 	* math/test-math-iscanonical.cc: New file.
> 	* sysdeps/ieee754/ldbl-96/bits/iscanonical.h (iscanonical):
> 	Provide a C++ implementation based on function overloading,
> 	rather than using __MATH_TG, which uses C-only builtins.
> 	* sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h (iscanonical):
> 	Likewise.
> 	* sysdeps/powerpc/powerpc64le/Makefile
> 	(CFLAGS-test-math-iscanonical.cc): New variable.

OK, but note

> +/* In C++ mode, __MATH_TG cannot be used, because it relies on
> +   __builtin_types_compatible_p, which is a C-only builtin.  On the
> +   other hand, overloading provides the means to distinguish between
> +   the floating-point types.  The overloading resolution will match
> +   the correct parameter (regardless of type qualifiers (i.e.: const
> +   and volatile).  */

(twice in this patch, and once already in math.h) is missing a second 
close parenthesis to match the first open parenthesis, and should be 
fixed.
Gabriel F. T. Gomes Oct. 3, 2017, 7:10 p.m. UTC | #2 
On 03 Oct 2017, Joseph Myers wrote:

>> +/* In C++ mode, __MATH_TG cannot be used, because it relies on
>> +   __builtin_types_compatible_p, which is a C-only builtin.  On the
>> +   other hand, overloading provides the means to distinguish between
>> +   the floating-point types.  The overloading resolution will match
>> +   the correct parameter (regardless of type qualifiers (i.e.: const
>> +   and volatile).  */  
>
>(twice in this patch, and once already in math.h) is missing a second 
>close parenthesis to match the first open parenthesis, and should be 
>fixed.

Thanks.  Pushed with these changes.
H.J. Lu Oct. 3, 2017, 11:59 p.m. UTC | #3 
On 10/3/17, Gabriel F. T. Gomes <gabriel@inconstante.eti.br> wrote:
> On 03 Oct 2017, Joseph Myers wrote:
>
>>> +/* In C++ mode, __MATH_TG cannot be used, because it relies on
>>> +   __builtin_types_compatible_p, which is a C-only builtin.  On the
>>> +   other hand, overloading provides the means to distinguish between
>>> +   the floating-point types.  The overloading resolution will match
>>> +   the correct parameter (regardless of type qualifiers (i.e.: const
>>> +   and volatile).  */
>>
>>(twice in this patch, and once already in math.h) is missing a second
>>close parenthesis to match the first open parenthesis, and should be
>>fixed.
>
> Thanks.  Pushed with these changes.
>
>

On i686 wit GCC 7, I got

test-math-iscanonical.cc: In function ‘void check_type()’:
test-math-iscanonical.cc:33:11: error: use of an operand of type
‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
     errors++;
           ^~
test-math-iscanonical.cc: In instantiation of ‘void check_type() [with
T = float]’:
test-math-iscanonical.cc:39:22:   required from here
test-math-iscanonical.cc:33:11: error: use of an operand of type
‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
     errors++;
     ~~~~~~^~
H.J. Lu Oct. 4, 2017, 12:01 a.m. UTC | #4 
On 10/3/17, H.J. Lu <hjl.tools@gmail.com> wrote:
> On 10/3/17, Gabriel F. T. Gomes <gabriel@inconstante.eti.br> wrote:
>> On 03 Oct 2017, Joseph Myers wrote:
>>
>>>> +/* In C++ mode, __MATH_TG cannot be used, because it relies on
>>>> +   __builtin_types_compatible_p, which is a C-only builtin.  On the
>>>> +   other hand, overloading provides the means to distinguish between
>>>> +   the floating-point types.  The overloading resolution will match
>>>> +   the correct parameter (regardless of type qualifiers (i.e.: const
>>>> +   and volatile).  */
>>>
>>>(twice in this patch, and once already in math.h) is missing a second
>>>close parenthesis to match the first open parenthesis, and should be
>>>fixed.
>>
>> Thanks.  Pushed with these changes.
>>
>>
>
> On i686 wit GCC 7, I got
>
> test-math-iscanonical.cc: In function ‘void check_type()’:
> test-math-iscanonical.cc:33:11: error: use of an operand of type
> ‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
>      errors++;
>            ^~
> test-math-iscanonical.cc: In instantiation of ‘void check_type() [with
> T = float]’:
> test-math-iscanonical.cc:39:22:   required from here
> test-math-iscanonical.cc:33:11: error: use of an operand of type
> ‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
>      errors++;
>      ~~~~~~^~
>

I am testing this:

diff --git a/math/test-math-iscanonical.cc b/math/test-math-iscanonical.cc
index aba68acb4f..8ced7a73b4 100644
--- a/math/test-math-iscanonical.cc
+++ b/math/test-math-iscanonical.cc
@@ -20,7 +20,7 @@
 #include <math.h>
 #include <stdio.h>

-static bool errors;
+static int errors;

 template <class T>
 static void
H.J. Lu Oct. 4, 2017, 12:43 a.m. UTC | #5 
On 10/3/17, H.J. Lu <hjl.tools@gmail.com> wrote:
> On 10/3/17, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On 10/3/17, Gabriel F. T. Gomes <gabriel@inconstante.eti.br> wrote:
>>> On 03 Oct 2017, Joseph Myers wrote:
>>>
>>>>> +/* In C++ mode, __MATH_TG cannot be used, because it relies on
>>>>> +   __builtin_types_compatible_p, which is a C-only builtin.  On the
>>>>> +   other hand, overloading provides the means to distinguish between
>>>>> +   the floating-point types.  The overloading resolution will match
>>>>> +   the correct parameter (regardless of type qualifiers (i.e.: const
>>>>> +   and volatile).  */
>>>>
>>>>(twice in this patch, and once already in math.h) is missing a second
>>>>close parenthesis to match the first open parenthesis, and should be
>>>>fixed.
>>>
>>> Thanks.  Pushed with these changes.
>>>
>>>
>>
>> On i686 wit GCC 7, I got
>>
>> test-math-iscanonical.cc: In function ‘void check_type()’:
>> test-math-iscanonical.cc:33:11: error: use of an operand of type
>> ‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
>>      errors++;
>>            ^~
>> test-math-iscanonical.cc: In instantiation of ‘void check_type() [with
>> T = float]’:
>> test-math-iscanonical.cc:39:22:   required from here
>> test-math-iscanonical.cc:33:11: error: use of an operand of type
>> ‘bool’ in ‘operator++’ is deprecated [-Werror=deprecated]
>>      errors++;
>>      ~~~~~~^~
>>
>
> I am testing this:
>
> diff --git a/math/test-math-iscanonical.cc b/math/test-math-iscanonical.cc
> index aba68acb4f..8ced7a73b4 100644
> --- a/math/test-math-iscanonical.cc
> +++ b/math/test-math-iscanonical.cc
> @@ -20,7 +20,7 @@
>  #include <math.h>
>  #include <stdio.h>
>
> -static bool errors;
> +static int errors;
>
>  template <class T>
>  static void
>
>

This is what I checked in.
Gabriel F. T. Gomes Oct. 4, 2017, 1:20 a.m. UTC | #6 
On 03 Oct 2017, H.J. Lu wrote:
>On 10/3/17, H.J. Lu <hjl.tools@gmail.com> wrote:
>> --- a/math/test-math-iscanonical.cc
>> +++ b/math/test-math-iscanonical.cc
>> @@ -20,7 +20,7 @@
>>  #include <math.h>
>>  #include <stdio.h>
>>
>> -static bool errors;
>> +static int errors;
>>
>>  template <class T>
>>  static void
>>
>>  
>
>This is what I checked in.

Thanks!
Joseph Myers Oct. 4, 2017, 1:15 p.m. UTC | #7 
On Wed, 4 Oct 2017, H.J. Lu wrote:

> This is what I checked in.

This fix doesn't seem to be on 2.26 branch, but needs to go there as the 
original patch went there.

I don't think using an int count of errors and returning it from do_test 
is a good coding pattern, because if the count reaches 77 it will result 
in a spurious UNSUPPORTED result.  Of course in this particular test it 
can't reach 77, but a better pattern is either a boolean error state (set 
to true rather than using ++, given the warning quoted here), or a count 
but with do_test returning errors != 0.
Florian Weimer Oct. 4, 2017, 1:47 p.m. UTC | #8 
On 10/04/2017 03:15 PM, Joseph Myers wrote:
> On Wed, 4 Oct 2017, H.J. Lu wrote:
> 
>> This is what I checked in.
> 
> This fix doesn't seem to be on 2.26 branch, but needs to go there as the
> original patch went there.
> 
> I don't think using an int count of errors and returning it from do_test
> is a good coding pattern, because if the count reaches 77 it will result
> in a spurious UNSUPPORTED result.  Of course in this particular test it
> can't reach 77, but a better pattern is either a boolean error state (set
> to true rather than using ++, given the warning quoted here), or a count
> but with do_test returning errors != 0.

Agreed.  Note that TEST_VERIFY allows the test to continue after a 
failure, and it also arranges for a non-zero exit status (even across 
fork, but currently not across dlopen).  It's usually a good alternative 
to such error variables.

Thanks,
Florian
H.J. Lu Oct. 4, 2017, 9:36 p.m. UTC | #9 
On 10/4/17, Florian Weimer <fweimer@redhat.com> wrote:
> On 10/04/2017 03:15 PM, Joseph Myers wrote:
>> On Wed, 4 Oct 2017, H.J. Lu wrote:
>>
>>> This is what I checked in.
>>
>> This fix doesn't seem to be on 2.26 branch, but needs to go there as the
>> original patch went there.
>>
>> I don't think using an int count of errors and returning it from do_test
>> is a good coding pattern, because if the count reaches 77 it will result
>> in a spurious UNSUPPORTED result.  Of course in this particular test it
>> can't reach 77, but a better pattern is either a boolean error state (set
>> to true rather than using ++, given the warning quoted here), or a count
>> but with do_test returning errors != 0.
>
> Agreed.  Note that TEST_VERIFY allows the test to continue after a
> failure, and it also arranges for a non-zero exit status (even across
> fork, but currently not across dlopen).  It's usually a good alternative
> to such error variables.
>
> Thanks,
> Florian
>

i am testing this patch and will backport these 2 patches to 2.26 branch.
diff mbox series

Patch

diff --git a/math/Makefile b/math/Makefile
index 6c8aa3e413..008eeb2d18 100644
--- a/math/Makefile
+++ b/math/Makefile
@@ -208,7 +208,8 @@  tests-internal = test-matherr test-matherr-2
 tests-static += atest-exp atest-sincos atest-exp2
 
 ifneq (,$(CXX))
-tests += test-math-isinff test-math-iszero test-math-issignaling
+tests += test-math-isinff test-math-iszero test-math-issignaling \
+	 test-math-iscanonical
 endif
 
 ifneq (no,$(PERL))
@@ -356,6 +357,7 @@  CFLAGS-test-signgam-ullong-init-static.c = -std=c99
 CFLAGS-test-math-isinff.cc = -std=gnu++11
 CFLAGS-test-math-iszero.cc = -std=gnu++11
 CFLAGS-test-math-issignaling.cc = -std=gnu++11
+CFLAGS-test-math-iscanonical.cc = -std=gnu++11
 
 CFLAGS-test-iszero-excess-precision.c = -fexcess-precision=standard
 CFLAGS-test-iseqsig-excess-precision.c = -fexcess-precision=standard
diff --git a/math/test-math-iscanonical.cc b/math/test-math-iscanonical.cc
new file mode 100644
index 0000000000..aba68acb4f
--- /dev/null
+++ b/math/test-math-iscanonical.cc
@@ -0,0 +1,48 @@ 
+/* Test for the C++ implementation of iscanonical.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define _GNU_SOURCE 1
+#include <math.h>
+#include <stdio.h>
+
+static bool errors;
+
+template <class T>
+static void
+check_type ()
+{
+  T val = 0;
+
+  /* Check if iscanonical is available in C++ mode (bug 22235).  */
+  if (iscanonical (val) == 0)
+    errors++;
+}
+
+static int
+do_test (void)
+{
+  check_type<float> ();
+  check_type<double> ();
+  check_type<long double> ();
+#if __HAVE_DISTINCT_FLOAT128
+  check_type<_Float128> ();
+#endif
+  return errors;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h b/sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h
index 7ddb368d26..1741aac048 100644
--- a/sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h
+++ b/sysdeps/ieee754/ldbl-128ibm/bits/iscanonical.h
@@ -37,5 +37,22 @@  extern int __iscanonicall (long double __x)
    conversion, before being discarded; in IBM long double, there are
    encodings that are not consistently handled as corresponding to any
    particular value of the type, and we return 0 for those.  */
-# define iscanonical(x) __MATH_TG ((x), __iscanonical, (x))
-#endif
+# ifndef __cplusplus
+#  define iscanonical(x) __MATH_TG ((x), __iscanonical, (x))
+# else
+/* In C++ mode, __MATH_TG cannot be used, because it relies on
+   __builtin_types_compatible_p, which is a C-only builtin.  On the
+   other hand, overloading provides the means to distinguish between
+   the floating-point types.  The overloading resolution will match
+   the correct parameter (regardless of type qualifiers (i.e.: const
+   and volatile).  */
+extern "C++" {
+inline int iscanonical (float __val) { return __iscanonicalf (__val); }
+inline int iscanonical (double __val) { return __iscanonical (__val); }
+inline int iscanonical (long double __val) { return __iscanonicall (__val); }
+#  if __HAVE_DISTINCT_FLOAT128
+inline int iscanonical (_Float128 __val) { return __iscanonicalf128 (__val); }
+#  endif
+}
+# endif /* __cplusplus */
+#endif /* __NO_LONG_DOUBLE_MATH */
diff --git a/sysdeps/ieee754/ldbl-96/bits/iscanonical.h b/sysdeps/ieee754/ldbl-96/bits/iscanonical.h
index 4a4f4ad024..366125a2cc 100644
--- a/sysdeps/ieee754/ldbl-96/bits/iscanonical.h
+++ b/sysdeps/ieee754/ldbl-96/bits/iscanonical.h
@@ -34,4 +34,21 @@  extern int __iscanonicall (long double __x)
    conversion, before being discarded; in extended precision, there
    are encodings that are not consistently handled as corresponding to
    any particular value of the type, and we return 0 for those.  */
-#define iscanonical(x) __MATH_TG ((x), __iscanonical, (x))
+#ifndef __cplusplus
+# define iscanonical(x) __MATH_TG ((x), __iscanonical, (x))
+#else
+/* In C++ mode, __MATH_TG cannot be used, because it relies on
+   __builtin_types_compatible_p, which is a C-only builtin.  On the
+   other hand, overloading provides the means to distinguish between
+   the floating-point types.  The overloading resolution will match
+   the correct parameter (regardless of type qualifiers (i.e.: const
+   and volatile).  */
+extern "C++" {
+inline int iscanonical (float __val) { return __iscanonicalf (__val); }
+inline int iscanonical (double __val) { return __iscanonical (__val); }
+inline int iscanonical (long double __val) { return __iscanonicall (__val); }
+# if __HAVE_DISTINCT_FLOAT128
+inline int iscanonical (_Float128 __val) { return __iscanonicalf128 (__val); }
+# endif
+}
+#endif /* __cplusplus */
diff --git a/sysdeps/powerpc/powerpc64le/Makefile b/sysdeps/powerpc/powerpc64le/Makefile
index 3fd9d9a715..f554a791b7 100644
--- a/sysdeps/powerpc/powerpc64le/Makefile
+++ b/sysdeps/powerpc/powerpc64le/Makefile
@@ -16,6 +16,7 @@  $(foreach suf,$(all-object-suffixes),%f128_r$(suf)): CFLAGS += -mfloat128
 $(foreach suf,$(all-object-suffixes),$(objpfx)test-float128%$(suf)): CFLAGS += -mfloat128
 $(foreach suf,$(all-object-suffixes),$(objpfx)test-ifloat128%$(suf)): CFLAGS += -mfloat128
 CFLAGS-libm-test-support-float128.c += -mfloat128
+CFLAGS-test-math-iscanonical.cc += -mfloat128
 CFLAGS-test-math-issignaling.cc += -mfloat128
 CFLAGS-test-math-iszero.cc += -mfloat128
 $(objpfx)test-float128% $(objpfx)test-ifloat128% $(objpfx)test-math-iszero: \