Implement Double.toStringAsFixed.

Implement Double.toStringAsFixed.

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

[floitsch, 2011-12-02 20:20:10]

For now only toStringAsFixed is implemented. The other conversions are however
prepared and would be trivial to complete (just wanted to avoid modifying the
code in 5 locations if you have comments).
I would propose completing them in another CL (once this one is done).

[Ivan Posva, 2011-12-02 21:09:29]

http://codereview.chromium.org/8728006/diff/3001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

http://codereview.chromium.org/8728006/diff/3001/runtime/lib/double.cc#newcode10
runtime/lib/double.cc:10: #include "vm/double_conversion.h"
Hard to review if the file is not here.

[floitsch, 2011-12-03 13:45:58]

Sorry about the missing file.
fyi: I get a lot of cpplint errors, so I'm not sure which of those I can ignore
or not (most of them were already there before I added my code).

http://codereview.chromium.org/8728006/diff/3001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

http://codereview.chromium.org/8728006/diff/3001/runtime/lib/double.cc#newcode10
runtime/lib/double.cc:10: #include "vm/double_conversion.h"
On 2011/12/02 21:09:29, Ivan Posva wrote:
> Hard to review if the file is not here.

Done.

[antonm, 2011-12-12 13:23:59]

DBC

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:44: //
Expect.equals("-1.1111111111111111e+21",
(-1111111111111111111111.0).toStringAsFixed(8));
may you now uncomment those FIXMEs, there are I believe 3 of them.

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:65: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
just FYI: those expectations were taken from v8 tests, so we may want to be
compatible with it.

[floitsch, 2011-12-13 16:49:02]

Enabled more tests. And some fixes.
Note: the language/ToStringAsFixedTest now fails because 
1111111111111111111111.0.toStringAsFixed(8)); redirects to toString which is not
yet using the library.
I will either disable the test (comment the lines), the whole test-file (in the
status-file), or implement toString before submitting this CL.
In either case the test should work soon.

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:44: //
Expect.equals("-1.1111111111111111e+21",
(-1111111111111111111111.0).toStringAsFixed(8));
On 2011/12/12 13:23:59, antonm wrote:
> may you now uncomment those FIXMEs, there are I believe 3 of them.

Done.

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:65: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
On 2011/12/12 13:23:59, antonm wrote:
> just FYI: those expectations were taken from v8 tests, so we may want to be
> compatible with it.

This change is on purpose. Afaik there is no good reason to hide -0.

[antonm, 2011-12-14 06:44:44]

Thanks a lot, Florian.

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:65: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
It's up to you.  For v8 we're bug-to-bug compatible with Safari, so it might be
worth to be bug-to-bug compatible JS backends in Dart case, but I am not by any
means insisting.

On 2011/12/13 16:49:02, floitsch wrote:
> On 2011/12/12 13:23:59, antonm wrote:
> > just FYI: those expectations were taken from v8 tests, so we may want to be
> > compatible with it.
> 
> This change is on purpose. Afaik there is no good reason to hide -0.

[floitsch, 2011-12-14 09:23:48]

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/12001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:65: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
On 2011/12/14 06:44:44, antonm wrote:
> It's up to you.  For v8 we're bug-to-bug compatible with Safari, so it might
be
> worth to be bug-to-bug compatible JS backends in Dart case, but I am not by
any
> means insisting.
We should be able to intercept the toString case on numbers and add the '-' for
-0.0. Might be slower, though.

> 
> On 2011/12/13 16:49:02, floitsch wrote:
> > On 2011/12/12 13:23:59, antonm wrote:
> > > just FYI: those expectations were taken from v8 tests, so we may want to
be
> > > compatible with it.
> > 
> > This change is on purpose. Afaik there is no good reason to hide -0.
>

[Ivan Posva, 2011-12-20 00:30:03]

Only looked at the code for ToStringAsFixed in the new double_conversion.cc
file: LGTM with comments.

http://codereview.chromium.org/8728006/diff/17001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

http://codereview.chromium.org/8728006/diff/17001/runtime/lib/double.cc#newco...
runtime/lib/double.cc:189: const Double& arg =
Double::CheckedHandle(arguments->At(0));
@srdjan: It would make sense to changes these spots to GET_NATIVE_RECEIVER.

http://codereview.chromium.org/8728006/diff/17001/runtime/lib/double.dart
File runtime/lib/double.dart (right):

http://codereview.chromium.org/8728006/diff/17001/runtime/lib/double.dart#new...
runtime/lib/double.dart:113: double _pow(double exponent) native "Double_pow";
Thanks!

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
File runtime/vm/double_conversion.cc (right):

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
runtime/vm/double_conversion.cc:65: if (!(isnan(d) || (kLowerBoundary < d && d <
kUpperBoundary))) {
You could add an early bailout for NaN and return a canonicalized string "NaN"
instead of going through the full machinery below. You could even test that
before entering into the native code on the Dart side.

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
runtime/vm/double_conversion.cc:68: if (!(kMinFractionDigits <= fraction_digits
&&
Might be personal style, but I prefer not to have large expressions negated.
Same above...

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:63: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
Does it really make sense to differentiate -0 and +0?

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:87: Expect.equals("234.2040",
234.20405.toStringAsFixed(4));
This looks wrong, why is this not 234.2041?

[floitsch, 2011-12-22 21:53:20]

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
File runtime/vm/double_conversion.cc (right):

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
runtime/vm/double_conversion.cc:65: if (!(isnan(d) || (kLowerBoundary < d && d <
kUpperBoundary))) {
On 2011/12/20 00:30:03, Ivan Posva wrote:
> You could add an early bailout for NaN and return a canonicalized string "NaN"
> instead of going through the full machinery below. You could even test that
> before entering into the native code on the Dart side.

Added early bailout in dart-land.
Done.

http://codereview.chromium.org/8728006/diff/17001/runtime/vm/double_conversio...
runtime/vm/double_conversion.cc:68: if (!(kMinFractionDigits <= fraction_digits
&&
On 2011/12/20 00:30:03, Ivan Posva wrote:
> Might be personal style, but I prefer not to have large expressions negated.
> Same above...

Done.

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
File tests/language/src/ToStringAsFixedTest.dart (right):

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:63: Expect.equals("-0",
(-0.0).toStringAsFixed(0));
On 2011/12/20 00:30:03, Ivan Posva wrote:
> Does it really make sense to differentiate -0 and +0?

We can discuss this offline if you want.
I have asked a lot of people what they would prefer (or what makes more sense to
them) and so far nobody preferred a merged -0/0. It is trivial to implement for
the conversion routine.

http://codereview.chromium.org/8728006/diff/17001/tests/language/src/ToString...
tests/language/src/ToStringAsFixedTest.dart:87: Expect.equals("234.2040",
234.20405.toStringAsFixed(4));
On 2011/12/20 00:30:03, Ivan Posva wrote:
> This looks wrong, why is this not 234.2041?
Because 234.20405 is only an approximation of the double value
234.2040499999999951797...

[floitsch, 2011-12-22 22:02:03]

Forgot to save a file.
Also disabled to test-lines.