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 #8816

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

Comments

@qaqz111
Copy link

@qaqz111 qaqz111 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.

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Aug 27, 2017

Same in desktop. Maybe @tannergooding knows.

Loading

@qaqz111
Copy link
Author

@qaqz111 qaqz111 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 #6207

Loading

@tannergooding
Copy link
Member

@tannergooding tannergooding 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.

Loading

@mazong1123
Copy link
Contributor

@mazong1123 mazong1123 commented Aug 31, 2017

Loading

@mazong1123
Copy link
Contributor

@mazong1123 mazong1123 commented Aug 31, 2017

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

Loading

@tannergooding
Copy link
Member

@tannergooding tannergooding 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.

Loading

@mazong1123
Copy link
Contributor

@mazong1123 mazong1123 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: https://github.com/dotnet/coreclr/issues/10390#issuecomment-289449394)?

Loading

@tannergooding
Copy link
Member

@tannergooding tannergooding 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.

Loading

@jkotas
Copy link
Member

@jkotas jkotas 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 #8793.

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

Agree.

Loading

@mazong1123
Copy link
Contributor

@mazong1123 mazong1123 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 #8793.

@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 dotnet/coreclr#12894 merged.

Loading

@tannergooding
Copy link
Member

@tannergooding tannergooding 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.

Loading

@tannergooding tannergooding self-assigned this Sep 11, 2018
@tannergooding
Copy link
Member

@tannergooding tannergooding commented Feb 1, 2019

Now that dotnet/coreclr#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.

Loading

@danmoseley danmoseley closed this Feb 4, 2019
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@tannergooding tannergooding removed their assignment May 26, 2020
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants