Implement Double.{toString, toStringAsExponential, toStringAsPrecision}

Implement Double.{toString, toStringAsExponential, toStringAsPrecision}

Also switch to -0.0.

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

[floitsch, 2012-01-25 00:27:10]

I will file a bug against co19 for "-0.0" and make sure that all components
work. (I had to abort the frog test-run because I ran out of time).
If you have a test.py line that tests the same thing as the build-bot please
send it to me, so I can make sure I don't break anything.

https://chromiumcodereview.appspot.com/9113043/diff/1/runtime/vm/double_conve...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/1/runtime/vm/double_conve...
runtime/vm/double_conversion.cc:58: bool DoubleToString(double d, String&
result) {
linter complains that result should be either "const String&" or "String*".
Do you want me to change it?

[Ivan Posva, 2012-01-26 00:35:39]

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:192: int fraction_digits_value = fraction_digits.Value();
ditto from below!

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:209: int fraction_digits_value = fraction_digits.Value();
Potentially losing bits here!

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:227: int precision_value = precision.Value();
ditto!

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/vm/double_co...
File runtime/vm/double_conversion.h (right):

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/vm/double_co...
runtime/vm/double_conversion.h:15: bool DoubleToString(double d, String&
result);
Please change this to return a RawString* and don't pass a non-const reference
argument:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

[floitsch, 2012-01-27 12:51:06]

PTAL.

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:192: int fraction_digits_value = fraction_digits.Value();
On 2012/01/26 00:35:39, Ivan Posva wrote:
> ditto from below!

Done.

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:209: int fraction_digits_value = fraction_digits.Value();
On 2012/01/26 00:35:39, Ivan Posva wrote:
> Potentially losing bits here!
The dart code already checked that the number was in range. But in case somebody
calls directly I added a check to have consistent behavior.

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/lib/double.c...
runtime/lib/double.cc:227: int precision_value = precision.Value();
On 2012/01/26 00:35:39, Ivan Posva wrote:
> ditto!

Done.

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/vm/double_co...
File runtime/vm/double_conversion.h (right):

https://chromiumcodereview.appspot.com/9113043/diff/2001/runtime/vm/double_co...
runtime/vm/double_conversion.h:15: bool DoubleToString(double d, String&
result);
On 2012/01/26 00:35:39, Ivan Posva wrote:
> Please change this to return a RawString* and don't pass a non-const reference
> argument:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

Done.

[Ivan Posva, 2012-02-02 23:21:36]

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:192: RawString* result = NULL;
You should not keep raw pointers lying around. This has potential for many
breakages in the code below because of . Also NULL is definitely the wrong thing
here, what it really converts into is a int with value 0.

String& result = String::Handle();
...
if (...) {
  result = DoubleToStringAsFixed(...);
}
if (result.IsNull()) {
...
}

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:211: RawString* result = NULL;
ditto

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:231: RawString* result = NULL;
ditto

[floitsch, 2012-02-05 15:28:21]

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:192: RawString* result = NULL;
On 2012/02/02 23:21:36, Ivan Posva wrote:
> You should not keep raw pointers lying around. This has potential for many
> breakages in the code below because of . Also NULL is definitely the wrong
thing
> here, what it really converts into is a int with value 0.

Fixed. But out of curiosity: was the current code broken, or just a potential
point of breakages in the future?
> 
> String& result = String::Handle();
> ...
> if (...) {
>   result = DoubleToStringAsFixed(...);
> }
> if (result.IsNull()) {
> ...
> }

done.

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:211: RawString* result = NULL;
On 2012/02/02 23:21:36, Ivan Posva wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:231: RawString* result = NULL;
On 2012/02/02 23:21:36, Ivan Posva wrote:
> ditto

Done.

[Ivan Posva, 2012-02-06 18:25:44]

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/5001/runtime/lib/double.c...
runtime/lib/double.cc:192: RawString* result = NULL;
On 2012/02/05 15:28:21, floitsch wrote:
> On 2012/02/02 23:21:36, Ivan Posva wrote:
> > You should not keep raw pointers lying around. This has potential for many
> > breakages in the code below because of . Also NULL is definitely the wrong
> thing
> > here, what it really converts into is a int with value 0.
> 
> Fixed. But out of curiosity: was the current code broken, or just a potential
> point of breakages in the future?
> > 
> > String& result = String::Handle();
> > ...
> > if (...) {
> >   result = DoubleToStringAsFixed(...);
> > }
> > if (result.IsNull()) {
> > ...
> > }
> 
> done.

It was broken since:
- Assigning NULL to a RawObject* is not what you think it is.
- Doing handle operations while holding a RawObject* can cause unexpected
behaviour.

I agree that the way the code was written it was highly unlikely to suffer from
the above points, but it is not a style we want to ever have in the VM as it
will come to haunt us later.

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
runtime/vm/double_conversion.cc:79: return NULL;
I am sorry I missed this on the first read of this code: NULL is not a legal
RawString*. Please use "String::null()" instead.

Explanation: NULL in C++ is defined to be a pointer with all bits being 0, which
maps to a Smi with value zero in Dart.

Since your code here and in related functions in this file do not match the
expectation in your fixed code "if result.IsNull()" it looks to me as if you
also need to add some tests for that scenario.

[floitsch, 2012-02-11 22:42:30]

Normally all calls to the toString functions are properly guarded so we should
not be able to trigger the code you mentioned.
It is just here for security.

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
runtime/vm/double_conversion.cc:79: return NULL;
On 2012/02/06 18:25:44, Ivan Posva wrote:
> I am sorry I missed this on the first read of this code: NULL is not a legal
> RawString*. Please use "String::null()" instead.
> 
> Explanation: NULL in C++ is defined to be a pointer with all bits being 0,
which
> maps to a Smi with value zero in Dart.
> 
> Since your code here and in related functions in this file do not match the
> expectation in your fixed code "if result.IsNull()" it looks to me as if you
> also need to add some tests for that scenario.

done.
Changed this occurrence to an ASSERT.

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
runtime/vm/double_conversion.cc:94: if (!status) return NULL;
changed to String::null().
status is supposed to always be true. Should I change this (here and in the
other conversion routines) to an ASSERT?

[floitsch, 2012-02-22 09:20:33]

ping

[Ivan Posva, 2012-02-27 14:47:13]

LGTM with comments.

-Ivan

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
runtime/vm/double_conversion.cc:94: if (!status) return NULL;
On 2012/02/11 22:42:30, floitsch wrote:
> changed to String::null().
> status is supposed to always be true. Should I change this (here and in the
> other conversion routines) to an ASSERT?

What will happen in production when the status is not true? Will we get a bad
string conversion (e.g. partial or empty string), a null string from the builder
or a crash?

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double....
runtime/lib/double.cc:202: if (result.IsNull()) {
I assume this test is here in case the DoubleToStringAsFixed failed for some
reason, otherwise a simple else would do.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double....
runtime/lib/double.cc:220: DoubleToStringAsExponential(d,
static_cast<int>(fraction_digits_value));
Style nit: Generally we break the line after the call and before the params, see
your code above on line 204.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/double_c...
runtime/vm/double_conversion.cc:19: bool DoubleToCString(double d, char* buffer,
int buffer_size,
Nit: We do either all parameters on new line or one parameter on its own line.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/object.c...
runtime/vm/object.cc:6090: char buffer[kBufferSize];
Please avoid stack allocated buffers that are being printed into under "user"
control. Instead you can allocate this straight zone and avoid the copying
around below.

[floitsch, 2012-02-27 19:55:40]

The changes look bigger than they are, because I rebased to test the changes. I
will send you the real diff per mail.

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/13003/runtime/vm/double_c...
runtime/vm/double_conversion.cc:94: if (!status) return NULL;
On 2012/02/27 14:47:14, Ivan Posva wrote:
> On 2012/02/11 22:42:30, floitsch wrote:
> > changed to String::null().
> > status is supposed to always be true. Should I change this (here and in the
> > other conversion routines) to an ASSERT?
> 
> What will happen in production when the status is not true? Will we get a bad
> string conversion (e.g. partial or empty string), a null string from the
builder
> or a crash?

The string builder only wraps the raw char-array to make verifications. If the
double-conversion fails the result would be partial or empty.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double....
runtime/lib/double.cc:202: if (result.IsNull()) {
On 2012/02/27 14:47:14, Ivan Posva wrote:
> I assume this test is here in case the DoubleToStringAsFixed failed for some
> reason, otherwise a simple else would do.

Done.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/lib/double....
runtime/lib/double.cc:220: DoubleToStringAsExponential(d,
static_cast<int>(fraction_digits_value));
On 2012/02/27 14:47:14, Ivan Posva wrote:
> Style nit: Generally we break the line after the call and before the params,
see
> your code above on line 204.

Done.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/double_c...
File runtime/vm/double_conversion.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/double_c...
runtime/vm/double_conversion.cc:19: bool DoubleToCString(double d, char* buffer,
int buffer_size,
On 2012/02/27 14:47:14, Ivan Posva wrote:
> Nit: We do either all parameters on new line or one parameter on its own line.

Done.

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/9113043/diff/24001/runtime/vm/object.c...
runtime/vm/object.cc:6090: char buffer[kBufferSize];
On 2012/02/27 14:47:14, Ivan Posva wrote:
> Please avoid stack allocated buffers that are being printed into under "user"
> control. Instead you can allocate this straight zone and avoid the copying
> around below.

Done.

[Ivan Posva, 2012-02-27 22:20:35]

LGTM -ip