Re: msvcrt: Fix bugs in math.c

[Piotr Caban]

Hi,

On 03/22/15 12:28, 陈正 wrote:
> - Fixed the problem of atan(INF)/tanh(INF)/exp(INF) causing errno EDOM
> - Fixed the error of _copysign(1., -0.) returning 1

Please don't send unrelated changes in one patch, please split it into 2 
patches.

The changes that remove errno setting from atan, tanh and exp looks good 
for me.
> float CDECL MSVCRT__copysignf( float num, float sign )
>   {
> - /* FIXME: Behaviour for Nan/Inf? */
> - if (sign < 0.0)
> -
>    return num < 0.0 ? num : -num;
> - return num < 0.0 ? -num : num;
> - /* FIXME: Behaviour for signbit(NAN) is different in Linux and
> -
> *        Windows, where Windows gives a zero for -NAN
> -
> */
> - if (signbit(sign))
> -
>    return signbit(num) ? num : -num;
> - return signbit(num) ? -num : num;
> }

Why don't you add an isnan() check? Is there any reason for returning 
"signbit(num) ? -num : num" instead of returning num?

It would be nice to also change signbit definition on systems that 
doesn't support it, so it at least works for normal numbers.

Thanks,
Piotr

msvcrt: Fix bugs in math.c

[Kevin Chan]

Hi Piotr,

Please don't send unrelated changes in one patch, please split it into 2
> patches.

Done. Thanks :)
> float CDECL MSVCRT__copysignf( float num, float sign )
> > {
> > - /* FIXME: Behaviour for Nan/Inf? */
> > - if (sign < 0.0)
> > -
> >    return num < 0.0 ? num : -num;
> > - return num < 0.0 ? -num : num;
> > - /* FIXME: Behaviour for signbit(NAN) is different in Linux and
> > -
> > *        Windows, where Windows gives a zero for -NAN
> > -
> > */
> > - if (signbit(sign))
> > -
> >    return signbit(num) ? num : -num;
> > - return signbit(num) ? -num : num;
> > }
>
> Why don't you add an isnan() check? Is there any reason for returning
> "signbit(num) ? -num : num" instead of returning num?

Linux and Windows treats NAN differently and I am not quite sure which I
should follow here, so I just leave it for now...
I think the _copysignf() is supposed to return a (-num) when (num < 0 &&
sign >0), so I think this "signbit(num) ? -num : num" might be right.
> It would be nice to also change signbit definition on systems that doesn't
> support it, so it at least works for normal numbers.

The signbit() function is actually part of C99, not my implementation, so I
think it should be supported on most systems? Though I am not quite sure...

Thanks,
Kevin

msvcrt: Fix bugs in math.c

[Piotr Caban]

On 03/22/15 15:02, Kevin Chan wrote:
> Why don't you add an isnan() check? Is there any reason for
> returning "signbit(num) ? -num : num" instead of returning num?
>
> Linux and Windows treats NAN differently and I am not quite sure which I
> should follow here, so I just leave it for now...

The goal is to make _copysign behave as on windows. The signbit function 
is not available in Visual Studio.
> I think the _copysignf() is supposed to return a (-num) when (num < 0 &&
> sign >0), so I think this "signbit(num) ? -num : num" might be right.

Sorry, I've misread it earlier. This part looks good.
> It would be nice to also change signbit definition on systems that
> doesn't support it, so it at least works for normal numbers.
>
> The signbit() function is actually part of C99, not my implementation,
> so I think it should be supported on most systems? Though I am not quite
> sure...

It's not available when compiled with Visual Studio. There's following 
code in math.c:
     #ifndef signbit
     #define signbit(x) 0
     #endif
I think it would be better if it's at least changed to something like:
#define signbit(x) ((x)<0 ? 1 : 0)
It will not work correctly for -NAN or -0 but it will not break the 
cases where old implementation was working.

Thanks,
Piotr

msvcrt: Fix bugs in math.c

[Zheng Chen]

Hi Piotr,

2015-03-22 23:45 GMT+08:00 Piotr Caban :
> On 03/22/15 15:02, Kevin Chan wrote:
> > Linux and Windows treats NAN differently and I am not quite sure which I
> > should follow here, so I just leave it for now...
>
> The goal is to make _copysign behave as on windows. The signbit function
> is not available in Visual Studio.

Understood. Thank you :)
> It would be nice to also change signbit definition on systems that
> > doesn't support it, so it at least works for normal numbers.
> >
> > The signbit() function is actually part of C99, not my implementation,
> > so I think it should be supported on most systems? Though I am not quite
> > sure...
>
> It's not available when compiled with Visual Studio. There's following
> code in math.c:
>     #ifndef signbit
>     #define signbit(x) 0
>     #endif
> I think it would be better if it's at least changed to something like:
> #define signbit(x) ((x)<0 ? 1 : 0)
> It will not work correctly for -NAN or -0 but it will not break the cases
> where old implementation was working.

Sorry I didn't notice my program in VS was linked to the C++ lib for this
signbit() function.
"#define signbit(x) ((x)<0 ? 1 : 0)"  Looks good to me, I will submit a
patch. Thanks for the advice :)
> Maybe we could rewrite this three-line code to "(signbit(sign) &&
> > signbit(num)) ? num : -num"? It seems more efficient, but it also seems
> > the code become not very easy-to-read...
>
> I would stick with more readable code.

Great! Me, too.

Thanks,
Zheng

msvcrt: Fix bugs in math.c

[Zheng Chen]

Hi Piotr,
> It would be nice to also change signbit definition on systems that
> > > doesn't support it, so it at least works for normal numbers.
> > >
> > > The signbit() function is actually part of C99, not my implementation,
> > > so I think it should be supported on most systems? Though I am not quite
> > > sure...
> >
> > It's not available when compiled with Visual Studio. There's following
> > code in math.c:
> >     #ifndef signbit
> >     #define signbit(x) 0
> >     #endif
> > I think it would be better if it's at least changed to something like:
> > #define signbit(x) ((x)<0 ? 1 : 0)
> > It will not work correctly for -NAN or -0 but it will not break the cases
> > where old implementation was working.
>
> Sorry I didn't notice my program in VS was linked to the C++ lib for this
> signbit() function.
> "#define signbit(x) ((x)<0 ? 1 : 0)"  Looks good to me, I will submit a
> patch. Thanks for the advice :)

Just came to me that maybe we should include isnan() in signbit() instead
of using isnan() in _copysign(), seems like that's how Windows does it.

Thanks,
Zheng
>

msvcrt: Fix bugs in math.c

[Piotr Caban]

On 03/22/15 17:54, Zheng Chen wrote:
> Just came to me that maybe we should include isnan() in signbit()
> instead of using isnan() in _copysign(), seems like that's how Windows
> does it.

As far as I can see on MSDN signbit is only available in VS2012 and 
VS2013, I don't see any exports related to it in msvcr*/msvcp* dlls, so 
I guess it's inlined.

We use host system implementation of the function to implement some 
other functions in wine. That's why you can't change signbit implementation.

Thanks,
Piotr

msvcrt: Fix bugs in math.c

[Zheng Chen]

2015-03-23 1:06 GMT+08:00 Piotr Caban :
> On 03/22/15 17:54, Zheng Chen wrote:
> > Just came to me that maybe we should include isnan() in signbit()
> > instead of using isnan() in _copysign(), seems like that's how Windows
> > does it.
>
> As far as I can see on MSDN signbit is only available in VS2012 and
> VS2013, I don't see any exports related to it in msvcr*/msvcp* dlls, so I
> guess it's inlined.
>
> We use host system implementation of the function to implement some other
> functions in wine. That's why you can't change signbit implementation.

Got it~ Thanks very much :)

Regards,
Zheng