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

[Pedro Alves]

On 11/14/2016 06:16 PM, Pedro Alves wrote:
> 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

Sorry, I wrote this this patch in a roundabout way, trying
different potential solutions, and the version I sent
somewhat in a hurry didn't simplify as much as possible
in the end...

If we rename the new macro to _GL_MATH_CXX_REAL_FLOATING_DECL_2
(and rename the old _GL_MATH_CXX_REAL_FLOATING_DECL_2 to
something else, making it a helper macro), then these ...

> +
>  /* 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)

... changes here (and similar places) shouldn't be necessary.

Let me try that and send a new patch.


-- 
Thanks,
Pedro Alves