RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
> -Original Message-
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
> Sent: Wednesday, April 23, 2008 9:21 AM
> To: [EMAIL PROTECTED]
> Subject: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
>
> Author: faridz
> Date: Wed Apr 23 08:20:07 2008
> New Revision: 650902
>
> URL: http://svn.apache.org/viewvc?rev=650902&view=rev
> Log:
> 2008-04-23 Farid Zaripov <[EMAIL PROTECTED]>
>
> STDCXX-880
> * src/num_put.cpp: Added simple overloads of the
> __rw_isfinite(),
> __rw_signbit(), __rw_isinf(), __rw_isnan(),
> __rw_isqnan(), __rw_issnan()
> for float and long double types.
>
> Modified:
> stdcxx/trunk/src/num_put.cpp
>
> Modified: stdcxx/trunk/src/num_put.cpp
> URL:
> http://svn.apache.org/viewvc/stdcxx/trunk/src/num_put.cpp?rev=
> 650902&r1=650901&r2=650902&view=diff
> ==
>
> --- stdcxx/trunk/src/num_put.cpp (original)
> +++ stdcxx/trunk/src/num_put.cpp Wed Apr 23 08:20:07 2008
> @@ -181,6 +181,36 @@
> #endif
>
>
> +inline bool __rw_isfinite (float) { return true; }
> +
> +inline bool __rw_signbit (float) { return false; }
> +
> +inline bool __rw_isinf (float) { return false; }
> +
> +inline bool __rw_isnan (float) { return false; }
> +
> +inline bool __rw_isqnan (float) { return false; }
> +
> +inline bool __rw_issnan (float) { return false; }
> +
> +
> +#ifndef _RWSTD_NO_LONG_DOUBLE
> +
> +inline bool __rw_isfinite (long double) { return true; }
> +
> +inline bool __rw_signbit (long double) { return false; }
> +
> +inline bool __rw_isinf (long double) { return false; }
> +
> +inline bool __rw_isnan (long double) { return false; }
> +
> +inline bool __rw_isqnan (long double) { return false; }
> +
> +inline bool __rw_issnan (long double) { return false; }
> +
> +#endif // _RWSTD_NO_LONG_DOUBLE
> +
> +
> static int
> __rw_fmat_infinite (char *buf, size_t bufsize, double val,
> unsigned flags)
> {
>
With just a quick review, this change looks a bit suspicious. The
respective overloads for type double are all defined within
conditional-compile directives. It follows that float and long double
overloads would be defined similarly unless I'm missing something.
Brad.
RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
> Eric Lemings wrote: > > >> -Original Message- >> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] >> >> Author: faridz >> Date: Wed Apr 23 08:20:07 2008 >> New Revision: 650902 >> >> URL: http://svn.apache.org/viewvc?rev=650902&view=rev >> Log: >> 2008-04-23 Farid Zaripov <[EMAIL PROTECTED]> >> >> STDCXX-880 >> * src/num_put.cpp: Added simple overloads of the >> __rw_isfinite(), __rw_signbit(), __rw_isinf(), >> __rw_isnan(), __rw_isqnan(), __rw_issnan() >> for float and long double types. >> >> Modified: >> stdcxx/trunk/src/num_put.cpp >> > > >With just a quick review, this change looks a bit suspicious. The >respective overloads for type double are all defined within >conditional-compile directives. It follows that float and long double >overloads would be defined similarly unless I'm missing something. > The overloads on double are in platform specific #ifdef blocks because they have platform specific code in them (_finite, finite, isfinite). At least that is the only reason I see for it. So I don't really see the overloads on float and double outside of platform specific blocks as a problem as there is no platform specific code in them. I'm more worried about why the overloads on float and long double are added at all. If they are only being called from __rw_fmat_infinite(), and that function only works with a double, then I see no motivation for the overloads. I'm also wondering why we provide default implementations of these functions if we know good and well that the results are wrong (for platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). Shouldn't we fail to compile in this case? The other thing that freaks me out is the implementation of __rw_signbit on platforms defining _RWSTD_OS_SUNOS. It assumes something similar to IEEE-745 double representation. This may be a safe assumption for the time being, but what happens if the user compiles with -fnonstd? Does this still work? >Brad. >
RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
>Travis Vitek wrote: > >The overloads on double are in platform specific #ifdef blocks because >they have platform specific code in them (_finite, finite, >isfinite). At >least that is the only reason I see for it. So I don't really see the >overloads on float and double outside of platform specific blocks as a >problem as there is no platform specific code in them. > >I'm more worried about why the overloads on float and long double are >added at all. If they are only being called from __rw_fmat_infinite(), >and that function only works with a double, then I see no >motivation for the overloads. > >I'm also wondering why we provide default implementations of these >functions if we know good and well that the results are wrong (for >platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). >Shouldn't we fail to compile in this case? As opposed to serializing bad results... >The other thing that freaks me out is the implementation of >__rw_signbit >on platforms defining _RWSTD_OS_SUNOS. It assumes something similar to >IEEE-745 double representation. This may be a safe assumption for the >time being, but what happens if the user compiles with -fnonstd? Does >this still work? > With all that said, the sum of the changes to num_put.cpp seem to eliminate the unexpected failures in the 22.locale.num.put test on both sunos-5.10-gcc-4.1.1 and aix-5.3-vacpp-8.0 12D builds. Travis
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
> From: Travis Vitek [mailto:[EMAIL PROTECTED] > To: dev@stdcxx.apache.org > Suject: RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp >>With just a quick review, this change looks a bit suspicious. The >>respective overloads for type double are all defined within >>conditional-compile directives. It follows that float and long double >>overloads would be defined similarly unless I'm missing something. >> > >The overloads on double are in platform specific #ifdef blocks because >they have platform specific code in them (_finite, finite, isfinite). At >least that is the only reason I see for it. So I don't really see the >overloads on float and double outside of platform specific blocks as a >problem as there is no platform specific code in them. Right. >I'm more worried about why the overloads on float and long double are >added at all. If they are only being called from __rw_fmat_infinite(), >and that function only works with a double, then I see no motivation for >the overloads. The float, double and long double versions of __rw_isfinite() are called from __rw_put_num() prior to call __rw_fmat_infinite(). >I'm also wondering why we provide default implementations of these >functions if we know good and well that the results are wrong (for >platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). >Shouldn't we fail to compile in this case? We need just convert NaN or Inf value to string. If we don't know the platform-specific functions or some platform doesn't provides such functions, we should fallback to using printf(). >The other thing that freaks me out is the implementation of __rw_signbit >on platforms defining _RWSTD_OS_SUNOS. It assumes something similar to >IEEE-745 double representation. This may be a safe assumption for the >time being, but what happens if the user compiles with -fnonstd? Does >this still work? Martin has changed the __rw_signbit() to not use signbit() on Solaris to avoid linking with libsunmath (and libm?) libraries. I see in documentation that copysign() is also present on Solaris. Can you check in what library located copysign()? Farid.
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Travis Vitek wrote: Thanks for the careful review! Eric Lemings wrote: -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Author: faridz Date: Wed Apr 23 08:20:07 2008 New Revision: 650902 URL: http://svn.apache.org/viewvc?rev=650902&view=rev Log: 2008-04-23 Farid Zaripov <[EMAIL PROTECTED]> STDCXX-880 * src/num_put.cpp: Added simple overloads of the __rw_isfinite(), __rw_signbit(), __rw_isinf(), __rw_isnan(), __rw_isqnan(), __rw_issnan() for float and long double types. Modified: stdcxx/trunk/src/num_put.cpp With just a quick review, this change looks a bit suspicious. The respective overloads for type double are all defined within conditional-compile directives. It follows that float and long double overloads would be defined similarly unless I'm missing something. The overloads on double are in platform specific #ifdef blocks because they have platform specific code in them (_finite, finite, isfinite). At least that is the only reason I see for it. So I don't really see the overloads on float and double outside of platform specific blocks as a problem as there is no platform specific code in them. I guess I don't understand why the float overloads are all unconditionally hardcoded: http://fisheye6.cenqua.com/browse/stdcxx/trunk/src/num_put.cpp?r=trunk#l184 I'm more worried about why the overloads on float and long double are added at all. If they are only being called from __rw_fmat_infinite(), and that function only works with a double, then I see no motivation for the overloads. Hmm. I thought Farid was saying __rw_fmat_infinite() was only being called with a double argument. It looks like it's potentially called with all three floating point arguments except the __rw_isfinite() function is hardcoded for float and long double to return false so __rw_fmat_infinite() is only called for doubles. So he was right, but the way it's done seems very confusing. Why do we bother to call __rw_isfinite(float) and __rw_isfinite(long double) at all when we know that they're hardcoded to return true? I'm also wondering why we provide default implementations of these functions if we know good and well that the results are wrong (for platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). Shouldn't we fail to compile in this case? My understanding is that the defaults will make the facet fall back on using sprintf(). Farid, can you confirm that? (I don't think we should ever fail to compile this code as long we have sprintf to fall back on). The other thing that freaks me out is the implementation of __rw_signbit on platforms defining _RWSTD_OS_SUNOS. It assumes something similar to IEEE-745 double representation. This may be a safe assumption for the time being, but what happens if the user compiles with -fnonstd? Does this still work? It should although I haven't tried it. The bit pattern doesn't change. (FWIW, I'm responsible for the function on Solaris). Here's the documentation for reference: http://docs.sun.com/app/docs/doc/819-5265/bjapp?a=view#bjarb Martin
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Farid Zaripov wrote: From: Travis Vitek [mailto:[EMAIL PROTECTED] To: dev@stdcxx.apache.org Suject: RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp With just a quick review, this change looks a bit suspicious. The respective overloads for type double are all defined within conditional-compile directives. It follows that float and long double overloads would be defined similarly unless I'm missing something. The overloads on double are in platform specific #ifdef blocks because they have platform specific code in them (_finite, finite, isfinite). At least that is the only reason I see for it. So I don't really see the overloads on float and double outside of platform specific blocks as a problem as there is no platform specific code in them. Right. I'm more worried about why the overloads on float and long double are added at all. If they are only being called from __rw_fmat_infinite(), and that function only works with a double, then I see no motivation for the overloads. The float, double and long double versions of __rw_isfinite() are called from __rw_put_num() prior to call __rw_fmat_infinite(). Right, but they don't seem to do anything useful on any platform. AFAICT, __rw_fmat_infinite() only ever actually gets called to format double Infinities and NaNs. floats are unconditionally formatted using sprintf() and so are long doubles unless they are the same size as double. Is that correct? And if so, why don't we format float infinities using __rw_fmat_infinite()? Or why don't we have a float and long double overload for it? I'm also wondering why we provide default implementations of these functions if we know good and well that the results are wrong (for platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). Shouldn't we fail to compile in this case? We need just convert NaN or Inf value to string. If we don't know the platform-specific functions or some platform doesn't provides such functions, we should fallback to using printf(). This makes sense to me. The other thing that freaks me out is the implementation of __rw_signbit on platforms defining _RWSTD_OS_SUNOS. It assumes something similar to IEEE-745 double representation. This may be a safe assumption for the time being, but what happens if the user compiles with -fnonstd? Does this still work? Martin has changed the __rw_signbit() to not use signbit() on Solaris to avoid linking with libsunmath (and libm?) libraries. I see in documentation that copysign() is also present on Solaris. Can you check in what library located copysign()? It's in libsunmath that we don't link with and don't want to start just to look at a few bits. Here's the background on the change(s): http://www.nabble.com/Re%3A-svn-commit%3A-r646396stdcxx-trunk-src-num_put.cpp-td16599410.html#a16599410 Martin
RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
> -Original Message- > From: Martin Sebor [mailto:[EMAIL PROTECTED] On Behalf Of Martin Sebor > Sent: Thursday, April 24, 2008 8:53 AM > To: dev@stdcxx.apache.org > Subject: Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp > > I guess I don't understand why the float overloads are all > unconditionally hardcoded: > http://fisheye6.cenqua.com/browse/stdcxx/trunk/src/num_put.cpp > ?r=trunk#l184 Because I thought that is too late to implement float overloads and check them on every supporting platforms. Anyway _C_float case branch is not used at the moment (__rw_put_num() used in num_put<>::_C_put(), and _C_put() used in num_put<>::do_put() but there is no float overload for do_put()). > > > > > I'm more worried about why the overloads on float and long > double are > > added at all. If they are only being called from > __rw_fmat_infinite(), > > and that function only works with a double, then I see no > motivation > > for the overloads. > > Hmm. I thought Farid was saying __rw_fmat_infinite() was only > being called with a double argument. It looks like it's > potentially called with all three floating point arguments > except the __rw_isfinite() function is hardcoded for float > and long double to return false so > __rw_fmat_infinite() is only called for doubles. So he was > right, but the way it's done seems very confusing. Why do we > bother to call > __rw_isfinite(float) and __rw_isfinite(long double) at all > when we know that they're hardcoded to return true? I suppose that we will implement float and long double overloads for the all platforms in 4.2.2... And calling inline __rw_isfinite() which returns false is something like #if 0 directive. > > > > > I'm also wondering why we provide default implementations of these > > functions if we know good and well that the results are wrong (for > > platforms that don't define _WIN32, _RWSTD_OS_SUNOS, or fpclassify). > > Shouldn't we fail to compile in this case? > > My understanding is that the defaults will make the facet > fall back on using sprintf(). Farid, can you confirm that? (I > don't think we should ever fail to compile this code as long > we have sprintf to fall back on). Right. Farid.
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Farid Zaripov wrote: -Original Message- From: Martin Sebor [mailto:[EMAIL PROTECTED] On Behalf Of Martin Sebor Sent: Thursday, April 24, 2008 8:53 AM To: dev@stdcxx.apache.org Subject: Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp I guess I don't understand why the float overloads are all unconditionally hardcoded: http://fisheye6.cenqua.com/browse/stdcxx/trunk/src/num_put.cpp ?r=trunk#l184 Because I thought that is too late to implement float overloads and check them on every supporting platforms. Anyway _C_float case branch is not used at the moment (__rw_put_num() used in num_put<>::_C_put(), and _C_put() used in num_put<>::do_put() but there is no float overload for do_put()). Okay, that makes me feel a little better because num_put must already convert floats to doubles. We should probably comment out the float branch in __rw_put_num(). So let me try to summarize the new behavior: Solaris Windows Other floatX XX double fmt fmt printf long doubleprintf fmt printf Xconverted to double by iostreams fmt formatted using __rw_fmat_infinite(double) printf formatted using sprintf() Did I get it right? Btw., is there a test for this other than 22.locale.num.put? Martin
RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
> -Original Message- > From: Martin Sebor [mailto:[EMAIL PROTECTED] > Sent: Thursday, April 24, 2008 6:02 PM > To: dev@stdcxx.apache.org > Subject: Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp > > So let me try to summarize the new behavior: > > Solaris Windows Other >floatX XX >double fmt fmt printf >long doubleprintf fmt printf > >Xconverted to double by iostreams >fmt formatted using __rw_fmat_infinite(double) >printf formatted using sprintf() > > Did I get it right? Yes, but you missed the column fpclassify (platforms where fpclassify macro is defined). Solaris Windows fpclassify Other floatX X X X double fmt fmtfmt printf long doubleprintf fmt printf printf > Btw., is there a test for this other than 22.locale.num.put? It seems, there is no such test. Farid.
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Okay, so are we all comfortable with this? Or does anyone want to argue to have this change reverted from 4.2.1? If there are no objections I will go ahead and create a release candidate tarball and get the vote rolling sometime later today. Martin Farid Zaripov wrote: -Original Message- From: Martin Sebor [mailto:[EMAIL PROTECTED] Sent: Thursday, April 24, 2008 6:02 PM To: dev@stdcxx.apache.org Subject: Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp So let me try to summarize the new behavior: Solaris Windows Other floatX XX double fmt fmt printf long doubleprintf fmt printf Xconverted to double by iostreams fmt formatted using __rw_fmat_infinite(double) printf formatted using sprintf() Did I get it right? Yes, but you missed the column fpclassify (platforms where fpclassify macro is defined). Solaris Windows fpclassify Other floatX X X X double fmt fmtfmt printf long doubleprintf fmt printf printf Btw., is there a test for this other than 22.locale.num.put? It seems, there is no such test. Farid.
RE: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Martin Sebor wrote: > >Okay, so are we all comfortable with this? Or does anyone >want to argue to have this change reverted from 4.2.1? > Yes, it seems fine. > >If there are no objections I will go ahead and create >a release candidate tarball and get the vote rolling >sometime later today. You mentioned in our previous conversation that you may need me to do the changelog, but never actually asked me to do it. Just a note that I have not done it. > >Martin
Re: svn commit: r650902 - /stdcxx/trunk/src/num_put.cpp
Travis Vitek wrote: Martin Sebor wrote: Okay, so are we all comfortable with this? Or does anyone want to argue to have this change reverted from 4.2.1? Yes, it seems fine. If there are no objections I will go ahead and create a release candidate tarball and get the vote rolling sometime later today. You mentioned in our previous conversation that you may need me to do the changelog, but never actually asked me to do it. Just a note that I have not done it. I'm working on it now, thanks. Martin
