Fix floating point issues on Windows.

Fix floating point issues on Windows.

BUG=dartbug.com/5407
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=15546

[aam-me, 2012-11-27 04:00:40]

Hi,

  My original intent was to fix dart2js
co19/LibTest/core/double/operator_remainder_A01_t04 test that was failing in my
Windows environment, and that led to changes that need to be done in VM.

  Besides one thing that Mads pointed out regarding non-compliant behavior of
fmod() function on Windows, I ended up also changing round() function so it
handles round_a01_t01.dart:34 test and wrapping MS VC++ atan2 function so it is
compliant with C99 standard with (+/-inf,+/-inf) arguments.

  co19/LibTest/math/exp_a01_t01 test passes on Windows, so I put it under
($system == linux || $system == macos) condition in co19-dart2js.status file.

  Please, take a look at the changes when you have a chance.

Thanks!

[Mads Ager (google), 2012-11-27 08:43:23]

Thanks for looking into this Alexander!

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/c9...
File runtime/platform/c99_support_win.h (left):

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/c9...
runtime/platform/c99_support_win.h:50: static inline double round(double x) {
This is a missing c99 method. Could you move this one back? Either we should
keep that one here or we should move all of them to floating_point.h. I would
prefer keeping this one here along with the other methods that are missing on
Windows and leave floating_point.h for dealing with differences between platform
provided methods.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:12: // Workaround MS fmod bugs. ECMA-262
says:
Please remove the reference to the JavaScript spec here. This is not JS
specific.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:31: return (intpart + 1);
I would remove the parenthesis here.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:35: return (intpart - 1);
Ditto.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:40: // Bring MSVC atan2 behavior in line
with standard.
Expand comment to state that MSVC atan2 does not agree with standard at
infinities.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:51: return
atans_at_infinities[x_negative][y_negative];
Please index with integers instead of booleans here.

[aam-me, 2012-11-28 00:45:34]

Thank you, Mads, for the review!

PTAL

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/c9...
File runtime/platform/c99_support_win.h (left):

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/c9...
runtime/platform/c99_support_win.h:50: static inline double round(double x) {
Done.
I moved it to c99_support_win.cc, though, so it is not inline method as I think
it is got quite big now. Is that what you had in mind?

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:12: // Workaround MS fmod bugs. ECMA-262
says:
On 2012/11/27 08:43:23, Mads Ager wrote:
> Please remove the reference to the JavaScript spec here. This is not JS
> specific.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:31: return (intpart + 1);
On 2012/11/27 08:43:23, Mads Ager wrote:
> I would remove the parenthesis here.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:35: return (intpart - 1);
On 2012/11/27 08:43:23, Mads Ager wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:40: // Bring MSVC atan2 behavior in line
with standard.
On 2012/11/27 08:43:23, Mads Ager wrote:
> Expand comment to state that MSVC atan2 does not agree with standard at
> infinities.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/7001/runtime/platform/fl...
runtime/platform/floating_point_win.cc:51: return
atans_at_infinities[x_negative][y_negative];
On 2012/11/27 08:43:23, Mads Ager wrote:
> Please index with integers instead of booleans here.

Done.

[Mads Ager (google), 2012-11-28 09:26:26]

Getting there. Thanks for iterating on this!

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
File runtime/platform/c99_support_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
runtime/platform/c99_support_win.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
Just move your new implementation to c99_support_win.h as a static inline
function?

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
File runtime/platform/c99_support_win.h (left):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
runtime/platform/c99_support_win.h:50: static inline double round(double x) {
We should still have the round method declared here. I think round is only used
in one place, so I would just put your new implementation in here as a static
inline.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:12: // Workaround MS fmod bugs. ECMA-262
says:
Please remove reference to ECMA-262.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:22: // Bring MSVC atan2 behavior in line
with standard.
I think your changes to this file got lost during upload. Please expand comment
to state that MSVC atan2 disagrees with standard at infinities.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:33: return
atans_at_infinities[x_negative][y_negative];
Could you index with ints instead of bools here.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
File runtime/platform/floating_point_win.h (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.h:10: double round(double x);
Please move this line back to c99_support.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/p...
File runtime/platform/platform_sources.gypi (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/p...
runtime/platform/platform_sources.gypi:11: 'floating_point_win.cc',
Don't you need c99_support_win.cc here? If we move that code to
c99_support_win.h we don't need to.

[aam-me, 2012-11-28 12:14:36]

Sorry about that, Mads!
Yes, somehow some changes got lost. I got some kind of hiccup in local git when
was committing changes locally. I guess I saw new c99_support_win.cc in the
changelist, and brushed that hiccup off. Will be more careful in the future.

PTAL when you have a chance.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
File runtime/platform/c99_support_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
runtime/platform/c99_support_win.cc:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2012/11/28 09:26:26, Mads Ager wrote:
> Just move your new implementation to c99_support_win.h as a static inline
> function?

Done.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
File runtime/platform/c99_support_win.h (left):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/c...
runtime/platform/c99_support_win.h:50: static inline double round(double x) {
Done.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:12: // Workaround MS fmod bugs. ECMA-262
says:
On 2012/11/28 09:26:26, Mads Ager wrote:
> Please remove reference to ECMA-262.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:22: // Bring MSVC atan2 behavior in line
with standard.
On 2012/11/28 09:26:26, Mads Ager wrote:
> I think your changes to this file got lost during upload. Please expand
comment
> to state that MSVC atan2 disagrees with standard at infinities.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/f...
runtime/platform/floating_point_win.cc:33: return
atans_at_infinities[x_negative][y_negative];
On 2012/11/28 09:26:26, Mads Ager wrote:
> Could you index with ints instead of bools here.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/p...
File runtime/platform/platform_sources.gypi (right):

https://chromiumcodereview.appspot.com/11411188/diff/10001/runtime/platform/p...
runtime/platform/platform_sources.gypi:11: 'floating_point_win.cc',
Yes, you're right.

[Mads Ager (google), 2012-11-29 09:36:36]

Thanks! Last round: a couple of style nit picks and we are ready to go!

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
File runtime/platform/floating_point.h (right):

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point.h:8: inline double modulo(double x, double y) {
return fmod(x, y); }
To make the naming more uniform, could you call this modulo_ieee as well?

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:13: // dividend is finite and divisor is
an infinity => result equals dividend
Nit picking: could you start with a capital letter and end with a period?

I like the list style you have below. Can you use that here as well:

 - Dividend is finite and divisor is an infinity => result equals dividend.

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:25: //  - If y is +inf(-infinity) and x
is -inf, +3*pi/4 (-3*pi/4) is returned.
Could you write out 'infinity' for all occurrences here and below.

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:35: int ndx_x = (cls_x & _FPCLASS_PINF)
!= 0? 0: 1;
Please add spaces and write out index:

int index_x = (...) != 0 ? 0 : 1;
int index_y = (...) != 0 ? 0 : 1;

[aam-me, 2012-11-29 12:39:53]

Thank you, Mads, for the comments.

PTAL when you have a chance!

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
File runtime/platform/floating_point.h (right):

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point.h:8: inline double modulo(double x, double y) {
return fmod(x, y); }
Made it fmod_ieee for consistency with atan2_ieee.
What do you think?

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:13: // dividend is finite and divisor is
an infinity => result equals dividend
On 2012/11/29 09:36:36, Mads Ager wrote:
> Nit picking: could you start with a capital letter and end with a period?
> 
> I like the list style you have below. Can you use that here as well:
> 
>  - Dividend is finite and divisor is an infinity => result equals dividend.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:25: //  - If y is +inf(-infinity) and x
is -inf, +3*pi/4 (-3*pi/4) is returned.
On 2012/11/29 09:36:36, Mads Ager wrote:
> Could you write out 'infinity' for all occurrences here and below.

Done.

https://chromiumcodereview.appspot.com/11411188/diff/6002/runtime/platform/fl...
runtime/platform/floating_point_win.cc:35: int ndx_x = (cls_x & _FPCLASS_PINF)
!= 0? 0: 1;
On 2012/11/29 09:36:36, Mads Ager wrote:
> Please add spaces and write out index:
> 
> int index_x = (...) != 0 ? 0 : 1;
> int index_y = (...) != 0 ? 0 : 1;

Done.

[Mads Ager (google), 2012-11-29 13:25:13]

LGTM, thanks Alexander. Let's get this landed! :)

https://chromiumcodereview.appspot.com/11411188/diff/3014/runtime/platform/fl...
File runtime/platform/floating_point_win.cc (right):

https://chromiumcodereview.appspot.com/11411188/diff/3014/runtime/platform/fl...
runtime/platform/floating_point_win.cc:35: // at (+/-1, +/-1). ndx_x is 0, when
x is +infinty, 1 when x is -infinty.
ndx_x and ndx_y -> index_x and index_y.

[aam-me, 2012-11-30 03:35:24]

Thank you, Mads, for the review!
The fix landed.

https://codereview.chromium.org/11411188/diff/3014/runtime/platform/floating_...
File runtime/platform/floating_point_win.cc (right):

https://codereview.chromium.org/11411188/diff/3014/runtime/platform/floating_...
runtime/platform/floating_point_win.cc:35: // at (+/-1, +/-1). ndx_x is 0, when
x is +infinty, 1 when x is -infinty.
On 2012/11/29 13:25:14, Mads Ager wrote:
> ndx_x and ndx_y -> index_x and index_y.

Done.