| Message ID | alpine.DEB.2.02.1210021055440.4891@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 2, 2012 at 3:57 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > (Forgot libstdc++...) > > > Hello, > > here is the patch from PR54686. Several notes: > > * I'll have to ask experts if std::abs(unsigned) (yes, a weird thing to do, > but still) is meant to return a double... don't we have a core issue about preferring unsigned -> long or long long? > * I still don't like the configure-time _GLIBCXX_USE_INT128, I think it > should use defined(__SIZEOF_INT128__), which would help other compilers. Why would that be a problem with the appropriate #define? > * newlib has llabs, according to the doc. It would be good to know what > newlib is missing for libstdc++ to detect it as C99-ready. > > I tested a previous version (without __STRICT_ANSI__) on x86_64-linux-gnu > and Oleg Endo did a basic check on sh/newlib. I'll do a last check after the > review (no point if the patch needs changing again). In general, I think I have a bias toward using compiler intrinsics, for which the compiler already has lot of knowledge about. > > 2012-10-02 Marc Glisse <marc.glisse@inria.fr> > > PR libstdc++/54686 > * include/c_std/cstdlib (abs(long long)): Define fallback whenever > we have long long but possibly not llabs. > (abs(long long)): Use llabs when available. > (abs(__int128)): Define when we have __int128. > (div(long long, long long)): Use lldiv. > * testsuite/26_numerics/headers/cstdlib/54686.c: New file. > > -- > Marc Glisse > > Index: include/c_std/cstdlib > =================================================================== > --- include/c_std/cstdlib (revision 191941) > +++ include/c_std/cstdlib (working copy) > @@ -130,20 +130,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > using ::strtoul; > using ::system; > #ifdef _GLIBCXX_USE_WCHAR_T > using ::wcstombs; > using ::wctomb; > #endif // _GLIBCXX_USE_WCHAR_T > > inline long > abs(long __i) { return labs(__i); } > > +#if defined (_GLIBCXX_USE_LONG_LONG) \ > + && (!_GLIBCXX_USE_C99 || _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC) > + // Fallback version if we don't have llabs but still allow long long. > + inline long long > + abs(long long __x) { return __x >= 0 ? __x : -__x; } > +#endif > + > +#if !defined(__STRICT_ANSI__) && defined(_GLIBCXX_USE_INT128) > + inline __int128 > + abs(__int128 __x) { return __x >= 0 ? __x : -__x; } > +#endif > + > inline ldiv_t > div(long __i, long __j) { return ldiv(__i, __j); } > > _GLIBCXX_END_NAMESPACE_VERSION > } // namespace > > #if _GLIBCXX_USE_C99 > > #undef _Exit > #undef llabs > @@ -161,29 +173,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > using ::lldiv_t; > #endif > #if _GLIBCXX_USE_C99_CHECK || _GLIBCXX_USE_C99_DYNAMIC > extern "C" void (_Exit)(int) throw () _GLIBCXX_NORETURN; > #endif > #if !_GLIBCXX_USE_C99_DYNAMIC > using ::_Exit; > #endif > > +#if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > inline long long > - abs(long long __x) { return __x >= 0 ? __x : -__x; } > + abs(long long __x) { return ::llabs (__x); } > > -#if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > using ::llabs; > > inline lldiv_t > div(long long __n, long long __d) > - { lldiv_t __q; __q.quot = __n / __d; __q.rem = __n % __d; return __q; } > + { return ::lldiv (__n, __d); } > > using ::lldiv; > #endif > > #if _GLIBCXX_USE_C99_LONG_LONG_CHECK || _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > extern "C" long long int (atoll)(const char *) throw (); > extern "C" long long int > (strtoll)(const char * __restrict, char ** __restrict, int) throw (); > extern "C" unsigned long long int > (strtoull)(const char * __restrict, char ** __restrict, int) throw (); > @@ -198,22 +210,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > _GLIBCXX_END_NAMESPACE_VERSION > } // namespace __gnu_cxx > > namespace std > { > #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > using ::__gnu_cxx::lldiv_t; > #endif > using ::__gnu_cxx::_Exit; > - using ::__gnu_cxx::abs; > #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC > + using ::__gnu_cxx::abs; > using ::__gnu_cxx::llabs; > using ::__gnu_cxx::div; > using ::__gnu_cxx::lldiv; > #endif > using ::__gnu_cxx::atoll; > using ::__gnu_cxx::strtof; > using ::__gnu_cxx::strtoll; > using ::__gnu_cxx::strtoull; > using ::__gnu_cxx::strtold; > } // namespace std > Index: testsuite/26_numerics/headers/cstdlib/54686.c > =================================================================== > --- testsuite/26_numerics/headers/cstdlib/54686.c (revision 0) > +++ testsuite/26_numerics/headers/cstdlib/54686.c (revision 0) > @@ -0,0 +1,32 @@ > +// { dg-do compile } > +// { dg-options "-std=c++11" } > + > +// Copyright (C) 2012 Free Software Foundation, Inc. > +// > +// This file is part of the GNU ISO C++ Library. This library is free > +// software; you can redistribute it and/or modify it under the > +// terms of the GNU General Public License as published by the > +// Free Software Foundation; either version 3, or (at your option) > +// any later version. > +// > +// This 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 General Public License for more details. > +// > +// You should have received a copy of the GNU General Public License along > +// with this library; see the file COPYING3. If not see > +// <http://www.gnu.org/licenses/>. > + > +#include <cmath> > +#include <cstdlib> > +#include <type_traits> > +#include <utility> > + > +#ifdef _GLIBCXX_USE_LONG_LONG > +void test01() > +{ > + static_assert (std::is_same<decltype (std::abs (std::declval<long long> > ())), > + long long>::value, "Missing abs(long long)"); > +} > +#endif > > Property changes on: testsuite/26_numerics/headers/cstdlib/54686.c > ___________________________________________________________________ > Added: svn:keywords > + Author Date Id Revision URL > Added: svn:eol-style > + native > >
On Tue, 2 Oct 2012, Gabriel Dos Reis wrote: > On Tue, Oct 2, 2012 at 3:57 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >> (Forgot libstdc++...) >> >> >> Hello, >> >> here is the patch from PR54686. Several notes: >> >> * I'll have to ask experts if std::abs(unsigned) (yes, a weird thing to do, >> but still) is meant to return a double... > > don't we have a core issue about preferring unsigned -> long or long long? Here I am talking of a library issue: the wording that says that there are sufficient overloads such that integer types call the double version of math functions. It is fairly obvious that it doesn't apply to abs(long) for instance which has an explicit overload. For short or unsigned, I still read it as saying that it converts to double... >> * I still don't like the configure-time _GLIBCXX_USE_INT128, I think it >> should use defined(__SIZEOF_INT128__), which would help other compilers. > > Why would that be a problem with the appropriate #define? The library installed by the system was compiled with g++, and is then used with clang++. If we can avoid installing 2 config.h files to make that work... >> * newlib has llabs, according to the doc. It would be good to know what >> newlib is missing for libstdc++ to detect it as C99-ready. >> >> I tested a previous version (without __STRICT_ANSI__) on x86_64-linux-gnu >> and Oleg Endo did a basic check on sh/newlib. I'll do a last check after the >> review (no point if the patch needs changing again). > > In general, I think I have a bias toward using compiler intrinsics, > for which the > compiler already has lot of knowledge about. More precisely, does that mean you want __builtin_llabs instead of ::llabs? I thought the compiler knew they were the same.
On Tue, Oct 2, 2012 at 4:21 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 2 Oct 2012, Gabriel Dos Reis wrote: > >> On Tue, Oct 2, 2012 at 3:57 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> >>> (Forgot libstdc++...) >>> >>> >>> Hello, >>> >>> here is the patch from PR54686. Several notes: >>> >>> * I'll have to ask experts if std::abs(unsigned) (yes, a weird thing to >>> do, >>> but still) is meant to return a double... >> >> >> don't we have a core issue about preferring unsigned -> long or long long? > > > Here I am talking of a library issue: the wording that says that there are > sufficient overloads such that integer types call the double version of math > functions. It is fairly obvious that it doesn't apply to abs(long) for > instance which has an explicit overload. For short or unsigned, I still read > it as saying that it converts to double... I understand that it is originally a library issue, but I don't think it makes sense to resolve it in isolation of that core issue. > > >>> * I still don't like the configure-time _GLIBCXX_USE_INT128, I think it >>> should use defined(__SIZEOF_INT128__), which would help other compilers. >> >> >> Why would that be a problem with the appropriate #define? > > > The library installed by the system was compiled with g++, and is then used > with clang++. If we can avoid installing 2 config.h files to make that > work... Two things: 1. that is clearly a clang problem. I don't think it is libstdc++'s job tp try to solve clang's misguided configuration and installation. 2. I am not sure you understand what I wrote: you can leave the use of the current macro the way it is if you appropriately define it in terms of what you want to change it to. > > >>> * newlib has llabs, according to the doc. It would be good to know what >>> newlib is missing for libstdc++ to detect it as C99-ready. >>> >>> I tested a previous version (without __STRICT_ANSI__) on x86_64-linux-gnu >>> and Oleg Endo did a basic check on sh/newlib. I'll do a last check after >>> the >>> review (no point if the patch needs changing again). >> >> >> In general, I think I have a bias toward using compiler intrinsics, >> for which the >> compiler already has lot of knowledge about. > > > More precisely, does that mean you want __builtin_llabs instead of ::llabs? > I thought the compiler knew they were the same. Yes. Another reason is that it simplifies the implementation AND if people want want to do something with the intrinsics' fallback libstdc++ will gracefully deliver that. -- Gaby
On Tue, 2 Oct 2012, Gabriel Dos Reis wrote: > I understand that it is originally a library issue, but I don't think > it makes sense to resolve it in isolation of that core issue. They seem mostly orthogonal to me, since the library only uses an informal language describing the desired outcome and not the actual overloads necessary to achieve it, whereas the core issue is about determining priorities for a non-ambiguous overload resolution (if we are talking about the same, where Jens Maurer has a proposal). >> The library installed by the system was compiled with g++, and is then used >> with clang++. If we can avoid installing 2 config.h files to make that >> work... > > Two things: > 1. that is clearly a clang problem. I don't think it is libstdc++'s job > tp try to solve clang's misguided configuration and installation. Translated: libstdc++ should only ever be used with the very version of g++ that was used to compile it. clang++, icpc, sunCC, etc should never try to use a libstdc++ compiled with another compiler. I am not saying libstdc++ should go to great lengths to support other compilers, but when it is actually easier to support them than not to... (testing a macro is easier than a configure test) > 2. I am not sure you understand what I wrote: you can leave the > use of the current macro the way it is if you appropriately > define it in terms of what you want to change it to. I was complaining about the configure-time nature of the macro. If it is defined at each compiler run based on __SIZEOF_INT128__, I am happy. >> More precisely, does that mean you want __builtin_llabs instead of ::llabs? >> I thought the compiler knew they were the same. > > Yes. Another reason is that it simplifies the implementation AND if > people want want to do something with the intrinsics' fallback > libstdc++ will gracefully deliver that. I don't see how that simplifies the implementation, it is several characters longer than ::llabs, and we still need to handle llabs. Or do you mean: always call __builtin_llabs (whether we have an llabs or not), and let the compiler replace it with either (x<0)?-x:x or a library call (I assume it never does that unless it has seen a corresponding declaration)? Note that I am happy to let you take over this PR if you like.
2012/10/2 Marc Glisse <marc.glisse@inria.fr>: > Here I am talking of a library issue: the wording that says that there are > sufficient overloads such that integer types call the double version of math > functions. It is fairly obvious that it doesn't apply to abs(long) for > instance which has an explicit overload. For short or unsigned, I still read > it as saying that it converts to double... This really looks like a problem of the Standard Library specification to me and a corresponding issue should be submitted. In fact the wording can be interpreted that mixing <cstdlib> with <cmath> would imply two different versions of std::abs(int) because of different required return types. I will prepare a corresponding submission to the LWG. - Daniel
On Tue, Oct 2, 2012 at 8:07 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> The library installed by the system was compiled with g++, and is then >>> used >>> with clang++. If we can avoid installing 2 config.h files to make that >>> work... >> >> >> Two things: >> 1. that is clearly a clang problem. I don't think it is libstdc++'s job >> tp try to solve clang's misguided configuration and installation. > > > Translated: libstdc++ should only ever be used with the very version of g++ > that was used to compile it. clang++, icpc, sunCC, etc should never try to > use a libstdc++ compiled with another compiler. Obviously, I cannot require you to exercise common sense and keep in check non-sensical strech. libstdc++ was and is developed for GCc/g++. If you are have a 3rd party compiler that you would like to use with g++/libstdc++, you should (a) either convince your 3rd party compiler supplier to understand the library you already have (libstdc++), or (b) supply yourself the glue between libstdc++ and your compiler. Many compilers have done that in the past; I don't see anything special with clang++ Whining on this list about libstdc++ internal macros and your dislike of them is not going to produce anything today or tomorrow. > > I am not saying libstdc++ should go to great lengths to support other > compilers, but when it is actually easier to support them than not to... > (testing a macro is easier than a configure test) > > >> 2. I am not sure you understand what I wrote: you can leave the >> use of the current macro the way it is if you appropriately >> define it in terms of what you want to change it to. > > > I was complaining about the configure-time nature of the macro. If it is > defined at each compiler run based on __SIZEOF_INT128__, I am happy. I am saying to can arrange to supply the appropriate definition without having to change the uses. > >>> More precisely, does that mean you want __builtin_llabs instead of >>> ::llabs? >>> I thought the compiler knew they were the same. >> >> >> Yes. Another reason is that it simplifies the implementation AND if >> people want want to do something with the intrinsics' fallback >> libstdc++ will gracefully deliver that. > > > I don't see how that simplifies the implementation, it is several characters > longer than ::llabs, and we still need to handle llabs. You are on the wrong track if you are taking the number of characters used in the implemetation. > Or do you mean: > always call __builtin_llabs (whether we have an llabs or not), and let the > compiler replace it with either (x<0)?-x:x or a library call (I assume it > never does that unless it has seen a corresponding declaration)? > > Note that I am happy to let you take over this PR if you like. > > -- > Marc Glisse
On Tue, Oct 2, 2012 at 9:34 AM, Daniel Krügler <daniel.kruegler@gmail.com> wrote: > 2012/10/2 Marc Glisse <marc.glisse@inria.fr>: >> Here I am talking of a library issue: the wording that says that there are >> sufficient overloads such that integer types call the double version of math >> functions. It is fairly obvious that it doesn't apply to abs(long) for >> instance which has an explicit overload. For short or unsigned, I still read >> it as saying that it converts to double... > > This really looks like a problem of the Standard Library specification > to me and > a corresponding issue should be submitted. In fact the wording can be > interpreted > that mixing <cstdlib> with <cmath> would imply two different versions of > std::abs(int) because of different required return types. I will > prepare a corresponding > submission to the LWG. This was already an issue I reported to LWG when C++98 came out. Now that you hold wrtite access to the issue document, you can make sure it won't slip through the crack this time :-p -- Gaby
On Tue, Oct 2, 2012 at 8:07 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > Or do you mean: > always call __builtin_llabs (whether we have an llabs or not), and let the > compiler replace it with either (x<0)?-x:x or a library call (I assume it > never does that unless it has seen a corresponding declaration)? See what we did in c/cmath and c_global/cmath. What you find there is the result of years of several iterations (including something similar to your earlier patch) all having issues in one way of another until we settled on the builtin functions approach. I have no appetite to go back to those days full of headache. -- Gaby
Index: include/c_std/cstdlib =================================================================== --- include/c_std/cstdlib (revision 191941) +++ include/c_std/cstdlib (working copy) @@ -130,20 +130,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using ::strtoul; using ::system; #ifdef _GLIBCXX_USE_WCHAR_T using ::wcstombs; using ::wctomb; #endif // _GLIBCXX_USE_WCHAR_T inline long abs(long __i) { return labs(__i); } +#if defined (_GLIBCXX_USE_LONG_LONG) \ + && (!_GLIBCXX_USE_C99 || _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC) + // Fallback version if we don't have llabs but still allow long long. + inline long long + abs(long long __x) { return __x >= 0 ? __x : -__x; } +#endif + +#if !defined(__STRICT_ANSI__) && defined(_GLIBCXX_USE_INT128) + inline __int128 + abs(__int128 __x) { return __x >= 0 ? __x : -__x; } +#endif + inline ldiv_t div(long __i, long __j) { return ldiv(__i, __j); } _GLIBCXX_END_NAMESPACE_VERSION } // namespace #if _GLIBCXX_USE_C99 #undef _Exit #undef llabs @@ -161,29 +173,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC using ::lldiv_t; #endif #if _GLIBCXX_USE_C99_CHECK || _GLIBCXX_USE_C99_DYNAMIC extern "C" void (_Exit)(int) throw () _GLIBCXX_NORETURN; #endif #if !_GLIBCXX_USE_C99_DYNAMIC using ::_Exit; #endif +#if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC inline long long - abs(long long __x) { return __x >= 0 ? __x : -__x; } + abs(long long __x) { return ::llabs (__x); } -#if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC using ::llabs; inline lldiv_t div(long long __n, long long __d) - { lldiv_t __q; __q.quot = __n / __d; __q.rem = __n % __d; return __q; } + { return ::lldiv (__n, __d); } using ::lldiv; #endif #if _GLIBCXX_USE_C99_LONG_LONG_CHECK || _GLIBCXX_USE_C99_LONG_LONG_DYNAMIC extern "C" long long int (atoll)(const char *) throw (); extern "C" long long int (strtoll)(const char * __restrict, char ** __restrict, int) throw (); extern "C" unsigned long long int (strtoull)(const char * __restrict, char ** __restrict, int) throw (); @@ -198,22 +210,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_END_NAMESPACE_VERSION } // namespace __gnu_cxx namespace std { #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC using ::__gnu_cxx::lldiv_t; #endif using ::__gnu_cxx::_Exit; - using ::__gnu_cxx::abs; #if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC + using ::__gnu_cxx::abs; using ::__gnu_cxx::llabs; using ::__gnu_cxx::div; using ::__gnu_cxx::lldiv; #endif using ::__gnu_cxx::atoll; using ::__gnu_cxx::strtof; using ::__gnu_cxx::strtoll; using ::__gnu_cxx::strtoull; using ::__gnu_cxx::strtold; } // namespace std Index: testsuite/26_numerics/headers/cstdlib/54686.c =================================================================== --- testsuite/26_numerics/headers/cstdlib/54686.c (revision 0) +++ testsuite/26_numerics/headers/cstdlib/54686.c (revision 0) @@ -0,0 +1,32 @@ +// { dg-do compile } +// { dg-options "-std=c++11" } + +// Copyright (C) 2012 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. +// +// This 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 General Public License for more details. +// +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <cmath> +#include <cstdlib> +#include <type_traits> +#include <utility> + +#ifdef _GLIBCXX_USE_LONG_LONG +void test01() +{ + static_assert (std::is_same<decltype (std::abs (std::declval<long long> ())), + long long>::value, "Missing abs(long long)"); +} +#endif