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 <pal...@redhat.com> > 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 <pal...@redhat.com> > > 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
