[PATCH] Fix real-floating argument functions in C++ mode

[Pedro Alves]

On 11/12/2016 05:30 PM, Paul Eggert wrote:
> Thanks, I installed your two recent patches. 

Thanks!

> I don't know C++ well;
> perhaps Bruno or another reviewer can double-check if they have the time.
> 
> I noticed that with the patches installed, the command './gnulib-tool
> --test --with-c++-tests frexp frexpl frexpf frexp-nolibm' fails with
> diagnostics like the following (Fedora 24, GCC 6.2.1 20160916)
> 
> make[3]: Entering directory
> '/home/eggert/src/gnu/gnulib/testdir4584/build/gltests'
> g++ -DHAVE_CONFIG_H -I. -I../../gltests  -DGNULIB_STRICT_CHECKING=1 -I.
> -I../../gltests -I.. -I../../gltests/.. -I../gllib
> -I../../gltests/../gllib -MT test-math-c++.o -MD -MP -MF
> .deps/test-math-c++.Tpo -c -o test-math-c++.o
> ../../gltests/test-math-c++.cc
> ../../gltests/test-math-c++.cc:37:45: error: invalid static_cast from
> type ‘<unresolved overloaded function type>’ to type ‘int (*)(float)’
>      = static_cast<rettype(*)parameters>(func)
>                                              ^
> ../../gltests/test-math-c++.cc:32:3: note: in expansion of macro
> ‘OVERLOADED_CHECK’
>    OVERLOADED_CHECK (func, rettype1, parameters1, _1); \
>    ^~~~~~~~~~~~~~~~
> 
> 
> However, similar failures occur even without the patches (the patches
> fix one of the failures, actually) so this is not a regression.

I looked at this today.

The problem here is that these functions (signbit, isfinite,
isnan, etc.) return bool in a conforming C++11 implementation:

  http://en.cppreference.com/w/cpp/numeric/math/signbit

Tweaking the test the obvious way to expect bool return types
makes the test pass with gcc >= 6.

However, if we _just_ do that, the test starts failing with gcc < 6,
which didn't have the new libstdc++ math.h wrapper that makes gcc
fully C++11 conforming.  We end up with gnulib providing the overloads as
returning int.  If we tweak the gnulib replacements to define the
functions as returning bool (with the obvious return type
change to _GL_MATH_CXX_REAL_FLOATING_DECL_1/_GL_MATH_CXX_REAL_FLOATING_DECL_2)),
we stumble on this on Fedora 23, at least:

make[3]: Entering directory 
'/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
Making all in .
make[4]: Entering directory 
'/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
g++ -std=gnu++11 -Wall -Wno-unused-variable -DHAVE_CONFIG_H -I. -I../../gltests 
 -DGNULIB_STRICT_CHECKING=1 -I. -I../../gltests -I.. -I../../gltests/.. 
-I../gllib -I../../gltests/../gllib    -MT test-math-c++.o -MD -MP -MF 
.deps/test-math-c++.Tpo -c -o test-math-c++.o ../../gltests/test-math-c++.cc
In file included from ../../gltests/test-math-c++.cc:22:0:
../gllib/math.h: In function ‘bool isinf(double)’:
../gllib/math.h:2422:1: error: ambiguating new declaration of ‘bool 
isinf(double)’
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
 ^
In file included from /usr/include/features.h:365:0,
                 from /usr/include/math.h:26,
                 from ../gllib/math.h:27,
                 from ../../gltests/test-math-c++.cc:22:
/usr/include/bits/mathcalls.h:201:1: note: old declaration ‘int isinf(double)’
 __MATHDECL_1 (int,isinf,, (_Mdouble_ __value)) __attribute__ ((__const__));
 ^
In file included from ../../gltests/test-math-c++.cc:22:0:
../gllib/math.h: In function ‘bool isnan(double)’:
../gllib/math.h:2540:1: error: ambiguating new declaration of ‘bool 
isnan(double)’
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
 ^

See <https://sourceware.org/bugzilla/show_bug.cgi?id=19439> for
background.

It seems to me that the spirit of gnulib should be to
define C/POSIX replacements, while largely moving out of the
way of the C++ runtime.  I don't think providing replacements
for C++ runtime holes (which is much larger than what
it inherits from C) should be in scope for gnulib.

So I think this should be addressed by defining the replacements
for these functions in the GNULIB_NAMESPACE namespace instead of
in the global namespace, like all other function replacements.
And given gnulib aims at C/POSIX, I think it's better/simpler if
the replacements follow the C interface, and thus return int, not
bool.

So, here's a patch that works for me on Fedora 23, tested with
gcc/g++ 4.7, 4.8, 4.9, 5.3.1 (system compiler) and 7 (trunk),
all in both c++03 and c++11 modes, using this script:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#!/bin/bash

set -e

test="./gnulib-tool --test --with-c++-tests frexp frexpl frexpf frexp-nolibm 
isnan isnanf isnand isnanl isfinite isinf"

WARN_CFLAGS="-Wall -Wno-unused-variable"

PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 
$WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++0x 
$WARN_CFLAGS" $test

PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 
$WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 
$WARN_CFLAGS" $test

PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 
$WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 
$WARN_CFLAGS" $test

# gcc 5.3.1 on Fedora 23
CC="gcc -Wall" CXX="g++ -std=gnu++03 -Wall -Wno-unused-variable" $test
CC="gcc -Wall" CXX="g++ -std=gnu++11 -Wall -Wno-unused-variable" $test

# gcc trunk on Fedora 23
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 
$WARN_CFLAGS" $test
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 
$WARN_CFLAGS" $test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

>From 0163126ec110fba504995a2aabebf09610cad1c4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <address@hidden>
Date: Mon, 14 Nov 2016 17:08:23 +0000
Subject: [PATCH] Fix real-floating argument functions in C++ mode

ChangeLog:
2016-11-14  Pedro Alves  <address@hidden>

        Fix real-floating argument functions in C++ mode.  Define define
        isfinite, isinf, isnan, signbit in the gnulib namespace instead of
        in the global namespace.
        * lib/math.in.h (_GL_MATH_CXX_REAL_FLOATING_DECL_NS): New macro.
        (isfinite, isinf, isnan, signbit): Use it.
        * tests/test-math-c++.cc: Test the isfinite, isinf, isnan, signbit
        overloads in the GNULIB_NAMESPACE namespace, instead of the global
        namespace.
---
 lib/math.in.h          | 34 ++++++++++++++++++++++++++++++----
 tests/test-math-c++.cc |  5 +++--
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/lib/math.in.h b/lib/math.in.h
index 9a194cb..8bbd25d 100644
--- a/lib/math.in.h
+++ b/lib/math.in.h
@@ -80,6 +80,16 @@ func (long double l)                                         
       \
 }
 #endif
 
+/* Define the FUNC overloads in the GNULIB_NAMESPACE namespace.  */
+#ifdef GNULIB_NAMESPACE
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func) \
+namespace GNULIB_NAMESPACE {                                        \
+_GL_MATH_CXX_REAL_FLOATING_DECL_2 (func)                            \
+}
+#else
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func)
+#endif
+
 /* Helper macros to define a portability warning for the
    classification macro FUNC called with VALUE.  POSIX declares the
    classification macros with an argument of real-floating (that is,
@@ -2044,10 +2054,14 @@ _GL_EXTERN_C int gl_isfinitel (long double x);
     gl_isfinitef (x))
 # endif
 # ifdef __cplusplus
-#  ifdef isfinite
+#  if defined isfinite || defined GNULIB_NAMESPACE
 _GL_MATH_CXX_REAL_FLOATING_DECL_1 (isfinite)
 #   undef isfinite
+#   ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isfinite)
+#   else
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (isfinite)
+#   endif
 #  endif
 # endif
 #elif defined GNULIB_POSIXCHECK
@@ -2071,10 +2085,14 @@ _GL_EXTERN_C int gl_isinfl (long double x);
     gl_isinff (x))
 # endif
 # ifdef __cplusplus
-#  ifdef isinf
+#  if defined isinf || defined GNULIB_NAMESPACE
 _GL_MATH_CXX_REAL_FLOATING_DECL_1 (isinf)
 #   undef isinf
+#   ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isinf)
+#   else
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
+#   endif
 #  endif
 # endif
 #elif defined GNULIB_POSIXCHECK
@@ -2189,10 +2207,14 @@ _GL_EXTERN_C int rpl_isnanl (long double x) 
_GL_ATTRIBUTE_CONST;
     __builtin_isnanf ((float)(x)))
 # endif
 # ifdef __cplusplus
-#  ifdef isnan
+#  if defined isnan || defined GNULIB_NAMESPACE
 _GL_MATH_CXX_REAL_FLOATING_DECL_1 (isnan)
 #   undef isnan
+#   ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isnan)
+#   else
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
+#   endif
 #  endif
 # else
 /* Ensure isnan is a macro.  */
@@ -2264,10 +2286,14 @@ _GL_EXTERN_C int gl_signbitl (long double arg);
     gl_signbitf (x))
 # endif
 # ifdef __cplusplus
-#  ifdef signbit
+#  if defined signbit || defined GNULIB_NAMESPACE
 _GL_MATH_CXX_REAL_FLOATING_DECL_1 (signbit)
 #   undef signbit
+#   ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (signbit)
+#   else
 _GL_MATH_CXX_REAL_FLOATING_DECL_2 (signbit)
+#   endif
 #  endif
 # endif
 #elif defined GNULIB_POSIXCHECK
diff --git a/tests/test-math-c++.cc b/tests/test-math-c++.cc
index f0f1448..cc7378c 100644
--- a/tests/test-math-c++.cc
+++ b/tests/test-math-c++.cc
@@ -24,7 +24,8 @@
 #include "signature.h"
 
 /* Signature check for a function that takes a real-floating argument.
-   Check that each overloaded function with the specified signature exists.  */
+   Check that each overloaded function with the specified signature
+   exists in the GNULIB_NAMESPACE namespace.  */
 #define REAL_FLOATING_CHECK(func,\
                             rettype1, parameters1,\
                             rettype2, parameters2,\
@@ -34,7 +35,7 @@
   OVERLOADED_CHECK (func, rettype3, parameters3, _3)
 #define OVERLOADED_CHECK(func, rettype, parameters, suffix) \
   static rettype (* _GL_UNUSED signature_check_ ## func ## suffix) parameters \
-    = static_cast<rettype(*)parameters>(func)
+    = static_cast<rettype(*)parameters>(GNULIB_NAMESPACE::func)
 
 
 /* Keep these checks in the same order as math.in.h!  */
-- 
2.5.5