Timespan int overloads #95779

stefannikolei · 2023-12-08T06:37:07Z

dotnet-issue-labeler · 2023-12-08T06:37:15Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-12-08T08:30:39Z

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	stefannikolei
Assignees:	-
Labels:	`new-api-needs-documentation`, `community-contribution`, `area-System.DateTime`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs

tarekgh · 2023-12-08T18:02:29Z

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs

+
+        public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0)
+        {
+            long totalTicks = (long)days * TicksPerDay


We need to ensure handling the overflow. Please have a look at the discussion #94143 (review) for more info. This applies to all APIs implementation here.

CC @tannergooding

tarekgh · 2023-12-12T17:54:23Z

@stefannikolei did you have a chance to look and address my comment #95779 (review)?

stefannikolei · 2023-12-12T18:20:55Z

@stefannikolei did you have a chance to look and address my comment #95779 (review)?

I thought I did. It perhaps can be that I misunderstood that discussion

tarekgh · 2023-12-12T18:45:46Z

I thought I did. It perhaps can be that I misunderstood that discussion

Can you try to call the newly introduced APIs with values that mathematically overflow? The expectation is to get overflow exception.

* Added new method IntervalFromTicks equivalent to IntervalFromDoubleTicks

…, int seconds, int milliseconds, int microseconds)

stephentoub · 2024-01-02T16:14:34Z

src/libraries/System.Linq.Expressions/tests/DebugViewTests.cs

@@ -310,13 +310,13 @@ public static void Call()
        {
            ParameterExpression x = Expression.Parameter(typeof(int), "x");
            ParameterExpression y = Expression.Parameter(typeof(int), "y");
-            ParameterExpression d = Expression.Parameter(typeof(double), "d");
+            ParameterExpression l = Expression.Parameter(typeof(long), "l");


Why is this test changing?

Because typeof(TimeSpan).GetMethod("FromSeconds", new[] { typeof(int) }) used to pick the overload which takes double but now with adding the new overloads, it will pick the one that takes long.

stephentoub · 2024-01-02T16:17:21Z

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs

+        /// <exception cref="OverflowException">
+        /// </exception>


Some of these exception blocks are empty

stephentoub · 2024-01-02T16:18:45Z

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs

+        public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0)
+        {
+            return FromDays(days)
+                + FromHours(hours)
+                + FromMinutes(minutes)
+                + FromSeconds(seconds)
+                + FromMilliseconds(milliseconds)
+                + FromMicroseconds(microseconds);
+        }


What happens if e.g. someone passes a days value that would overflow but then a negative hours value that would bring it back into the valid range? With this implementation that will still throw... is that the desired behavior?

This is an excellent question. We may implement this inside checked block and do the math manually by converting all units to ticks and adding them. I know we'll sacrifice some perf, but we'll have the correct behavior.

Personally I think that we should only throw if the final tick count is out of range.

It's always annoying when x + y + z fails but x + z + y would succeed and I think it provides a better experience if we treat FromDays as its own higher order thing.

tarekgh · 2024-01-02T19:56:49Z

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs

+        /// </exception>
+        public static TimeSpan FromDays(int days)
+        {
+            return FromMicroseconds(days * MicrosecondsPerDay);


days * MicrosecondsPerDay

This multiplication by itself can overflow and make the operation succeed without exception, I guess.

For example, using 1067519900 days will result -1008547758080 microseconds which is going to be considered as a valid value while it is really overflowed.
The best thing that can be done here is to check the day's value and directly convert the result to ticks if it is valid.

This comment applies to similar calling of From operations with other units (hours, minutes...etc.)

CC @tannergooding

Should we validate that it's less than MaxDays (10_675_199)? 1_067_519_900 is much larger

Yes, that is what I was suggesting when I said the best thing that can be done here is to check the day's value and directly convert the result to ticks if it is valid.

tarekgh · 2024-01-12T21:43:16Z

@stefannikolei checking if you'll have a chance to address the feedback?

ghost · 2024-01-27T03:01:45Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2024-02-10T06:01:34Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

tcortega added 5 commits October 28, 2023 18:52

Implements new intand long overloads for TimeSpan

c78cde4

Closes dotnet#93890

Refactors code and fixes build errors

7a24f9c

Addresses StyleCop errors

bfc99de

Removes trailing whitespace to fix StyleCop error

8148be4

Merge branch 'main' into timespan-int-overloads

556484c

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 8, 2023

dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Dec 8, 2023

stefannikolei added 3 commits December 8, 2023 07:38

Merge branch 'dotnet:main' into timespan-int-overloads

ddd9fc4

Remove From Milliseconds

cbf3aa0

Readd missing code

99d3a7d

huoyaoyuan added area-System.DateTime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 8, 2023

build-analysis bot mentioned this pull request Dec 8, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

Try Fix DebugViewTests

041174f

build-analysis bot mentioned this pull request Dec 8, 2023

Interop tests crash on Mono #95792

Closed

tarekgh added this to the 9.0.0 milestone Dec 8, 2023

tarekgh reviewed Dec 8, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs Show resolved Hide resolved

tarekgh reviewed Dec 8, 2023

View reviewed changes

Try fix failing DebugViewTest

6662a9c

build-analysis bot mentioned this pull request Dec 9, 2023

Test infra failure on iOS.Device.Aot.Test.WorkItemExecution CI leg #92792

Closed

3 tasks

stefannikolei added 2 commits December 10, 2023 08:06

Update implementation add xml comments

b5b4435

Add test for From overloads

01b5f43

stefannikolei marked this pull request as ready for review December 10, 2023 12:54

build-analysis bot mentioned this pull request Dec 10, 2023

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

stefannikolei added 4 commits December 13, 2023 18:52

Merge branch 'master' into timespan-int-overloads

a3828b9

Check for Overflows when creating

d65d75f

* Added new method IntervalFromTicks equivalent to IntervalFromDoubleTicks

Add OverFlow Test for FromMinutes

c36f110

use same logic as inthis constructor(int days, int hours, int minutes…

445ad4b

…, int seconds, int milliseconds, int microseconds)

This was referenced Dec 13, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

Segmentation fault in System.Text.RegularExpressions.Tests #93206

Closed

stephentoub reviewed Jan 2, 2024

View reviewed changes

tarekgh reviewed Jan 2, 2024

View reviewed changes

tarekgh added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 12, 2024

ghost added the no-recent-activity label Jan 27, 2024

ghost closed this Feb 10, 2024

tommysor mentioned this pull request Feb 18, 2024

New TimeSpan.From overloads which take integers #98633

Merged

github-actions bot locked and limited conversation to collaborators Mar 12, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timespan int overloads #95779

Timespan int overloads #95779

stefannikolei commented Dec 8, 2023 •

edited by jkotas

dotnet-issue-labeler bot commented Dec 8, 2023

ghost commented Dec 8, 2023

tarekgh Dec 8, 2023 •

edited

tarekgh commented Dec 12, 2023

stefannikolei commented Dec 12, 2023

tarekgh commented Dec 12, 2023

stephentoub Jan 2, 2024

tarekgh Jan 2, 2024 •

edited

stephentoub Jan 2, 2024

stephentoub Jan 2, 2024

tarekgh Jan 2, 2024

tannergooding Jan 2, 2024

tarekgh Jan 2, 2024 •

edited

tannergooding Jan 2, 2024

tarekgh Jan 2, 2024

tarekgh commented Jan 12, 2024

ghost commented Jan 27, 2024

ghost commented Feb 10, 2024

Timespan int overloads #95779

Timespan int overloads #95779

Conversation

stefannikolei commented Dec 8, 2023 • edited by jkotas

dotnet-issue-labeler bot commented Dec 8, 2023

ghost commented Dec 8, 2023

tarekgh Dec 8, 2023 • edited

Choose a reason for hiding this comment

tarekgh commented Dec 12, 2023

stefannikolei commented Dec 12, 2023

tarekgh commented Dec 12, 2023

stephentoub Jan 2, 2024

Choose a reason for hiding this comment

tarekgh Jan 2, 2024 • edited

Choose a reason for hiding this comment

stephentoub Jan 2, 2024

Choose a reason for hiding this comment

stephentoub Jan 2, 2024

Choose a reason for hiding this comment

tarekgh Jan 2, 2024

Choose a reason for hiding this comment

tannergooding Jan 2, 2024

Choose a reason for hiding this comment

tarekgh Jan 2, 2024 • edited

Choose a reason for hiding this comment

tannergooding Jan 2, 2024

Choose a reason for hiding this comment

tarekgh Jan 2, 2024

Choose a reason for hiding this comment

tarekgh commented Jan 12, 2024

ghost commented Jan 27, 2024

ghost commented Feb 10, 2024

stefannikolei commented Dec 8, 2023 •

edited by jkotas

tarekgh Dec 8, 2023 •

edited

tarekgh Jan 2, 2024 •

edited

tarekgh Jan 2, 2024 •

edited