Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

double.Parse(double.MinValue.ToString()) and double.Parse(double.MaxValue.ToString()) throw an System.OverflowException #13615

Closed
qaqz111 opened this issue Aug 27, 2017 · 12 comments

Comments

@qaqz111
Copy link

commented Aug 27, 2017

double.Parse(double.MinValue.ToString());
//or
double.Parse("-1.79769313486232E+308");

//and

double.Parse(double.MaxValue.ToString());
//or
double.Parse("1.79769313486232E+308");

These codes will throw a System.OverflowException which says "the value is too big or too small as double", while float with float.MaxValue and float.MinValue does not.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Aug 27, 2017

Same in desktop. Maybe @tannergooding knows.

@qaqz111

This comment has been minimized.

Copy link
Author

commented Aug 27, 2017

I've found an old related issue, looks like it's been noticed and scheduled.

  • Convert.ChangeType throws exception when converting Double.MaxValue #5989
@tannergooding

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

@danmosemsft, there are multiple bugs tracking in accuracies with the double.Parse() and double.ToString() methods.

If you want to round-trip, you must use double.ToString("G17") (R has its own bugs and does not ensure round-tripping will work).

There are some active PRs that fix some issues and some discussions on what to do about R (its been broken for a very long time, so the discussions are around whether taking the break is worthwhile).

I'll see if I can find the related PRs/issues and tag them here in a bit.

@mazong1123

This comment has been minimized.

Copy link
Collaborator

commented Aug 31, 2017

@tannergooding I think it's easy for me to find these PRs and issues :)

#13140
#13106
#10651 (comment)

@mazong1123

This comment has been minimized.

Copy link
Collaborator

commented Aug 31, 2017

We'd better handle the edge cases(MaxValue/MinValue) inside parse instead of asking user to use round-trip specifier.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

@mazong1123, thanks. This completely slipped my radar.

We should probably just start this by moving to the Roslyn RealParser (https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/RealParser.cs) which has been confirmed to be precise/valid according to the IEEE spec.

@mazong1123

This comment has been minimized.

Copy link
Collaborator

commented Sep 1, 2017

@tannergooding my understanding is this ToString implementation is following IEEE spec, so now we just need to re-write the parsing part. Is that correct?

Do we plan to implement RealParser in native part of coreclr or totally rewrite the whole tostring/parsing logic in managed code (I saw this is our eventual goal: #10390 (comment))?

@tannergooding

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Yes, I believe this ToString implementation implementation is compliant, although it might be worthwhile to run the RandomRealParserTests.sln suite the compiler has to help validate that.

Do we plan to implement RealParser in native part of coreclr...

Writing it in managed is the better long term goal (IMO), but it probably depends on the perf numbers as well. The current RealParser implementation, while in managed, looks like it incurs several heap allocations and isn't very well optimized for speed.

@jkotas

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Writing it in managed is the better long term goal

Yes, we want to have all parsing and formatting in C#. See #13544.

The current RealParser implementation, while in managed, isn't very well optimized for speed

Agree.

@mazong1123

This comment has been minimized.

Copy link
Collaborator

commented Sep 1, 2017

to run the RandomRealParserTests.sln suite the compiler has to help validate that.

@tannergooding is RandomRealParserTests.sln included in CI or we need to run it manually?

Yes, we want to have all parsing and formatting in C#. See #13544.

@jkotas seems like @stephentoub has already started working on this issue (if not I can help working on it).

I think we can re-write ToString() in managed code as well after #12894 merged.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@mazong1123, based on the comments, it needs to be run manually. It sounds like it runs a lot of validation and could take some time to complete.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Now that #22040 is merged, for .NET Core 3.0, ToString(), ToString("G"), ToString("R"), double.ToString("G17"), and float.ToString("G9") should now always produce a string that will roundtrip to the original value..

I will keep this open until an explicit testcase for double.MinValue and double.MaxValue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.