TimeSpan.FromMilliseconds(TimeSpan.MaxValue.TotalMilliseconds)

[ezverev]

TimeSpan.FromMilliseconds(TimeSpan.MaxValue.TotalMilliseconds) gives exception. I suggest the fix.

The problem is discussed here: https://connect.microsoft.com/VisualStudio/feedback/details/542235/timespan-structure-incorrectly-handles-values-close-to-min-and-max-value

The existing overflow checks are ignorant and superfluous.

[danmoseley]

Thanks for the contribution @ezverev we'll take a look.
Usually it's a good idea to open an issue (in corefx repo) before creating a PR so we can agree we want to take the change.

[ezverev]

@danmosemsft OK, noted.

[danmoseley]

@tarekgh

[tarekgh]

@joshfree the proposed change looks good to me as it will do the rounding to the ticks instead of the milliseconds which would be more precise. I am seeing your comment on https://connect.microsoft.com/VisualStudio/feedback/details/542235/timespan-structure-incorrectly-handles-values-close-to-min-and-max-value

Do you still have a concern?

I already checked manually the changes with values around TimeSpan.MaxValue and TimeSpan.MinValue and not seeing any problem.

Hello e_zverev,
Thanks for the post. Because of the loss of precision of the Double data type, this conversion can generate an OverflowException for values that are near to but still in the range of either MinValue or MaxValue. For example, this causes an OverflowException in the following attempt to instantiate a TimeSpan object:

// http://msdn.microsoft.com/en-us/library/system.timespan.frommilliseconds(VS.95).aspx
// The following throws an OverflowException at runtime
TimeSpan maxSpan = TimeSpan.FromMilliseconds(TimeSpan.MaxValue.TotalMilliseconds);

For more information on Double precision, please see http://en.wikipedia.org/wiki/Double_precision

Sincerely,
Josh Free

@ezverev could you please resolve the merge conflict?

[tarekgh]

@ezverev please resolve the merge conflict. Thanks.

[tarekgh]

@ezverev did you have a chance to resolve the conflict?

[ezverev]

Sorry for the delay. Resolved.

[tarekgh]

Thanks @ezverev for submitting this fix

[jkotas]

@tarekgh Do we need a regression test in corefx?

[jkotas]

Opened https://github.com/dotnet/corefx/issues/17631

[tarekgh]

@jkotas we already have tests

https://github.com/dotnet/corefx/blob/30e9fb02d048e88c71f772eed80ccff407f614d9/src/System.Runtime/tests/System/TimeSpanTests.cs

[ezverev]

I will look into appending a test in order to assure there is no longer unneeded precision loss when constructing TimeSpan from a double of different scales.

Thanks for the constructive approach. I am sorry to rush the thing and passing over the expected "issue discussion".

[stephentoub]

This appears to be breaking a bunch a bunch of corefx tests:
dotnet/corefx#17648

[danmoseley]

@ezverev that's totally fine and it takes time to get used to get used to the "ways" of any OSS project. We're glad for your contribution and hopefully you will be interested to make more in future.

https://github.com/dotnet/corefx/issues/17619 may be interesting.

[ezverev]

Working on the unit tests for modified TimeSpan I found a huge flaw in my fix. I got into the kid's trap with the overflow check. Existing unit tests do not cover it:

With my fix the TimeSpan.FromMilliseconds(922337203685477.62) gives incorrect result.

Please do revert the change that I've made. I will have to work on the issue further and enhance the unit tests before I resubmit the PR.