Fix TimeSpan parsing

[tarekgh]

This PR is addressing the issues discussed in the closed PR #21077

Summary

We are inconsistent when parsing the fraction part of the time spans especially when having a leading zeros. inconsistency means sometime we throw exception on some fraction string while not throwing in some other similar fractions.
Also, in some cases we parse the fraction incorrectly. here is some examples of the problems in the TimeSpan parsing;

• Parsing "0:00:00.00123456" (8-digits) return the same result as parsing "0:00:00.0123456" which is wrong. The first should produce 12346 and the second should produce 123456.
• "0:00:00.00000098" (8-digits) is parsed wrongly to 98 ticks while is should be parsed to 10 ticks (rounding it to 7-digits).
• "0:00:00.00000099" (8-digits) throwing exception while it should be parsed to to 10 ticks (rounding it to 7-digits without throwing).
• Parsing "0:0:0.00000001" (8-digits) return same result when parsing "01:01:01.0000001" (7 digits).
• parsing "0:00:00.01000000" (8-digits) produces 1000000 ticks while it should be 100000.

The Fix

The fix here is to make our behavior consistent and produce logical and expected results. The fix is when there is leading zeros, we'll round the number to 7-digits and succeed the parsing. here is the expectation of the cases mentioned above:

• Parsing "0:00:00.00123456" (8-digits) will produce 12346 ticks while parsing "0:00:00.0123456" will produce 123456 ticks.
• "0:00:00.00000098" (8-digits) will produce 10 ticks instead of 98 ticks.
• "0:00:00.00000099" (8-digits) will produce 10 ticks instead of 98 ticks and will not throw.
• Parsing "0:0:0.00000001" (8-digits) will produce 0 ticks while "01:01:01.0000001" (7 digits) will produce 1 tick.
• parsing "0:00:00.01000000" (8-digits) produces will produce 100000 ticks instead of 1000000 ticks.

Notes

• We still be limited to Max value 9,999,999 for the fraction. this is not changed and we should fail as we used to if we encounter a bigger number.
• We still limited of parsing numbers up to 0xFFFFFFF when parsing even if we have leading zeros. this is original design and we are keeping it. we can relax that if we find evidence it is needed.
• As we are fixing the parsing, it is kind of breaking change if anyone depended on the wrong behavior

[tarekgh]

@joshfree This is the TimeSpan parsing issue I talked to you before. please have a look and let me know if you have any comment or concern.

CC @Quole @krwq @danmosemsft

[tarekgh]

by the way, I am going to submit a test PR in corefx too.

[pengowray]

We still be limited to Max value 9,999,999 for the fraction. this is not changed and we should fail as we used to if we encounter a bigger number.

Bit confused by this as you also say you'll accept 8-digit fractions? edit: Oh... I guess you mean you will store 7-digits, but can read more?

Also, if you're going to accept 8 digits, I would suggest upping it to 9 digits to be compatible with anything out there which produces nanosecond precision output, such as protobuff.

[tarekgh]

@Quole

Bit confused by this as you also say you'll accept 8-digit fractions? edit: Oh... I guess you mean you will store 7-digits, but can read more?

9,999,999 is the max fraction value we can accept, we can read more digits only when having leading zeros (e.g. 09999999) and we'll round it at that time.

Also, if you're going to accept 8 digits, I would suggest upping it to 9 digits to be compatible with anything out there which produces nanosecond precision output, such as protobuff.

We'll accept more than 8 digits as long as the parsed number (without the leading zeros) not exceeding 0xFFFFFFF. if you look at the test PR dotnet/corefx#34561 you'll see I have added a test case like

yield return new object[] {"00:00:00.0268435455", new TimeSpan(268435)};

which is 10 digits.

[tarekgh]

@jkotas there is some corefx tests are expected to fail because of this change https://ci.dot.net/job/dotnet_coreclr/job/master/job/x64_release_windows_nt_corefx_innerloop_prtest/5479/ but I have addressing that in the test PR dotnet/corefx#34561 how usually we handle this cases in such PR's? is there a way disabling this tests for coreclr runs only? or is there a way to fix the used tests in the coreclr CI?

[danmoseley]

@tarekgh you can disable them in coreclr like this
https://github.com/dotnet/coreclr/pull/21645/files

[tarekgh]

Thanks @danmosemsft

[tarekgh]

@joshfree @krwq please let me know if you have any comment here. Thanks.

[tarekgh]

test Tizen armel Cross Checked Innerloop Build and Test

[tarekgh]

Thanks @joshfree for your review.

[jkotas]

@Anipik Could you please take a look at the mirror? It does not seem to be picking up this change.

[Anipik]

Done :)