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

[API Proposal]: Expose the remaining const values in TimeSpan #94545

Closed
IDisposable opened this issue Nov 8, 2023 · 35 comments · Fixed by #103498 or #103993
Closed

[API Proposal]: Expose the remaining const values in TimeSpan #94545

IDisposable opened this issue Nov 8, 2023 · 35 comments · Fixed by #103498 or #103993
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@IDisposable
Copy link
Contributor

IDisposable commented Nov 8, 2023

Edited By @tarekgh

This proposal has been approved before #94545 (comment). There was some feedback from @tannergooding and @IDisposable.

@tannergooding said: the concern I raised is that there are scenarios this can cause problems since days * TimeSpan.HoursPerDay could overflow. For example, if days > 89,478,485 then this will overflow to a negative int.
HoursPerDay happens to be the only case where it is "sensible" for it to be int, since both MaxDays (10,675,199) and MaxHours (256,204,778) are within int.MaxValue. That is also why the signatures are FromDays(int days, int hours, long minutes, long milliseconds, long microseconds). So other values, like MinutesPerDay (1440), MinutesPerHour (60), or even the existing MillisecondsPerTick (10,000) are all expected to be used in contexts where long is the target type.
When considering TimeSpan, which is where these constants are defined, hours and days are int, all other units are long.


Background and motivation

In the TimeSpan class there are a few public const values that are generally applicable that are usefully exposed.

There are also a bunch more that are internal const but would be generally useful for other code and are not more implementation driven than the already exposed const values.

These values should be exposed publicly on the TimeSpan class.

Note: NOT suggesting we expose the truly internal const values that are related to MinTicks and MaxTicks. They could be included but are just not as naturally useful elsewhere.

API Proposal

namespace System.Collections.Generic;

public class TimeSpan: // elided details
{
        // more elision

        public const long MicrosecondsPerMillisecond = TicksPerMillisecond / TicksPerMicrosecond; //           1,000
        public const long MicrosecondsPerSecond = TicksPerSecond / TicksPerMicrosecond;           //       1,000,000
        public const long MicrosecondsPerMinute = TicksPerMinute / TicksPerMicrosecond;           //      60,000,000
        public const long MicrosecondsPerHour = TicksPerHour / TicksPerMicrosecond;               //   3,600,000,000
        public const long MicrosecondsPerDay = TicksPerDay / TicksPerMicrosecond;                 //  86,400,000,000

        public const long MillisecondsPerSecond = TicksPerSecond / TicksPerMillisecond;           //           1,000
        public const long MillisecondsPerMinute = TicksPerMinute / TicksPerMillisecond;           //          60,000
        public const long MillisecondsPerHour = TicksPerHour / TicksPerMillisecond;               //       3,600,000
        public const long MillisecondsPerDay = TicksPerDay / TicksPerMillisecond;                 //      86,400,000

        public const long SecondsPerMinute = TicksPerMinute / TicksPerSecond;                     //              60
        public const long SecondsPerHour = TicksPerHour / TicksPerSecond;                         //           3,600
        public const long SecondsPerDay = TicksPerDay / TicksPerSecond;                           //          86,400

        public const long MinutesPerHour = TicksPerHour / TicksPerMinute;                         //              60
        public const long MinutesPerDay = TicksPerDay / TicksPerMinute;                           //           1,440

        public const long HoursPerDay = TicksPerDay / TicksPerHour;                               //              24
}

API Usage

if (TimeSpan.MinutesPerHour * numHours) > TimeSpan.MinutesPerDay * 7)
    Console.WriteLine("That's really weeks");

Alternative Designs

N/A

Risks

Since these are compile-time constant values, there would be no runtime cost in either memory or execution overhead. Additionally they would centralize basic real-world facts to one logical location (TimeSpan).

@IDisposable IDisposable added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 8, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 8, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 8, 2023
@vcsjones vcsjones 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 Nov 9, 2023
@ghost
Copy link

ghost commented Nov 9, 2023

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

Issue Details

Background and motivation

In the TimeSpan class there are a few public const values that are generally applicable that are usefully exposed.

There are also a bunch more that are internal const but would be generally useful for other code and are not more implementation driven than the already exposed const values.

These values should be exposed publicly on the TimeSpan class.

Note: NOT suggesting we expose the truly internal const values that are related to MinTicks and MaxTicks. They could be included but are just not as naturally useful elsewhere.

API Proposal

namespace System.Collections.Generic;

public class TimeSpan: // elided details
{
        // more elision

        public const long MicrosecondsPerMillisecond = TicksPerMillisecond / TicksPerMicrosecond; //           1,000
        public const long MicrosecondsPerSecond = TicksPerSecond / TicksPerMicrosecond;           //       1,000,000
        public const long MicrosecondsPerMinute = TicksPerMinute / TicksPerMicrosecond;           //      60,000,000
        public const long MicrosecondsPerHour = TicksPerHour / TicksPerMicrosecond;               //   3,600,000,000
        public const long MicrosecondsPerDay = TicksPerDay / TicksPerMicrosecond;                 //  86,400,000,000

        public const long MillisecondsPerSecond = TicksPerSecond / TicksPerMillisecond;           //           1,000
        public const long MillisecondsPerMinute = TicksPerMinute / TicksPerMillisecond;           //          60,000
        public const long MillisecondsPerHour = TicksPerHour / TicksPerMillisecond;               //       3,600,000
        public const long MillisecondsPerDay = TicksPerDay / TicksPerMillisecond;                 //      86,400,000

        public const long SecondsPerMinute = TicksPerMinute / TicksPerSecond;                     //              60
        public const long SecondsPerHour = TicksPerHour / TicksPerSecond;                         //           3,600
        public const long SecondsPerDay = TicksPerDay / TicksPerSecond;                           //          86,400

        public const long MinutesPerHour = TicksPerHour / TicksPerMinute;                         //              60
        public const long MinutesPerDay = TicksPerDay / TicksPerMinute;                           //           1,440

        public const long HoursPerDay = TicksPerDay / TicksPerHour;                               //              24
}

API Usage

if (TimeSpan.MinutesPerHour * numHours) > TimeSpan.MinutesPerDay * 7)
    Console.WriteLine("That's really weeks");

Alternative Designs

N/A

Risks

Since these are compile-time constant values, there would be no runtime cost in either memory or execution overhead. Additionally they would centralize basic real-world facts to one logical location (TimeSpan).

Author: IDisposable
Assignees: -
Labels:

api-suggestion, untriaged, area-System.DateTime

Milestone: -

@tarekgh tarekgh added this to the Future milestone Nov 9, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 9, 2023
IDisposable added a commit to IDisposable/dotnet-runtime that referenced this issue Nov 11, 2023
Make the useful constants that TimeSpan has internally declare public because they're rooted in hard reality and not changeable, and quite useful elsewhere.
Fixes dotnet#94545
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Nov 11, 2023
IDisposable added a commit to IDisposable/dotnet-runtime that referenced this issue Nov 11, 2023
Make the useful constants that TimeSpan has internally declare public because they're rooted in hard reality and not changeable, and quite useful elsewhere.
Fixes dotnet#94545
@IDisposable
Copy link
Contributor Author

Is this something we can add to the API review?

@tannergooding
Copy link
Member

CC. @tarekgh, this is a relatively simple ask and I know I've had to redefine such constants myself in personal projects to help with various extension methods or other time related structs used (such as a Timestamp struct used to track frame timings in a game engine).

@tarekgh
Copy link
Member

tarekgh commented May 9, 2024

I am fine to proceed with the proposal. I don't have any feedback as the proposal looks good to me as it is. I'll mark this as ready for review.

@tarekgh tarekgh added the api-ready-for-review API is ready for review, it is NOT ready for implementation label May 9, 2024
@tarekgh tarekgh modified the milestones: Future, 9.0.0 May 9, 2024
@IDisposable
Copy link
Contributor Author

Should I reopen the PR or start a new one? :)

@tarekgh
Copy link
Member

tarekgh commented May 9, 2024

@IDisposable please wait till the issue gets reviewed by the design committee and then we can reopen the PR. Thanks!

@bartonjs
Copy link
Member

bartonjs commented Jun 13, 2024

Video

  • The consts that can be int should be
  • Otherwise, looks good as proposed.
namespace System.Collections.Generic;

public partial class TimeSpan
{
        public const int MicrosecondsPerMillisecond = TicksPerMillisecond / TicksPerMicrosecond; //           1,000
        public const int MicrosecondsPerSecond = TicksPerSecond / TicksPerMicrosecond;           //       1,000,000
        public const int MicrosecondsPerMinute = TicksPerMinute / TicksPerMicrosecond;           //      60,000,000
        public const long MicrosecondsPerHour = TicksPerHour / TicksPerMicrosecond;               //   3,600,000,000
        public const long MicrosecondsPerDay = TicksPerDay / TicksPerMicrosecond;                 //  86,400,000,000

        public const int MillisecondsPerSecond = TicksPerSecond / TicksPerMillisecond;           //           1,000
        public const int MillisecondsPerMinute = TicksPerMinute / TicksPerMillisecond;           //          60,000
        public const int MillisecondsPerHour = TicksPerHour / TicksPerMillisecond;               //       3,600,000
        public const int MillisecondsPerDay = TicksPerDay / TicksPerMillisecond;                 //      86,400,000

        public const int SecondsPerMinute = TicksPerMinute / TicksPerSecond;                     //              60
        public const int SecondsPerHour = TicksPerHour / TicksPerSecond;                         //           3,600
        public const int SecondsPerDay = TicksPerDay / TicksPerSecond;                           //          86,400

        public const int MinutesPerHour = TicksPerHour / TicksPerMinute;                         //              60
        public const int MinutesPerDay = TicksPerDay / TicksPerMinute;                           //           1,440

        public const int HoursPerDay = TicksPerDay / TicksPerHour;                               //              24
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 13, 2024
@julealgon
Copy link

What was the underlying reason to not expose these when they were first introduced? Was it just a matter of YAGNI, or was there more to it?

@IDisposable
Copy link
Contributor Author

@bartonjs

The consts that can be int should be

In general, I certainly agree, I was just adjusting the visibility of what was already there... so moving them to int from long would have to be carefully checked in the current consumers of these internal values to make sure that this change doesn't accidentally introduce cases where binary operations that were dealing with an int combined with one of these long constants wouldn't break (overflow/etc.) if it's now ant int op int. I will ensure I've got tests for that in the final PR

@tarekgh
Copy link
Member

tarekgh commented Jun 13, 2024

What was the underlying reason to not expose these when they were first introduced? Was it just a matter of YAGNI, or was there more to it?

Usually, such constants get introduced when implementing other approved features. To expose anything, it needs to go as a separate proposal to design review it. That is what happened here.

@tannergooding
Copy link
Member

so moving them to int from long would have to be carefully checked

If we find this is problematic, then please comment back and we can keep them as long with the annotation that they need to be so that regular usage doesn't cause overflow or other problems.

I can easily get the sign-off for that over e-mail and just chalk it up to a consideration no one thought about in the 60s it took to review the APIs 😆

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

@bartonjs

I merged the changes but I missed your comment #94545 (comment). We went with long types for the new constants but if you think HoursPerDay should be int we can submit another PR to change that. Please let me know and sorry for missing your comment.

CC @tannergooding @terrajobst

@IDisposable
Copy link
Contributor Author

IDisposable commented Jun 19, 2024

Preparing a change to make the smaller constants int instead of long per this comment but given the concerns about accidental overflows, and other feedback maybe push back on keeping these two as long as well (as they're the most likely to unintentionally overflow) to parallel what was done with the MicrosecondsPerHour and MicrosecondsPerDay in the revised proposal

        public const long MillisecondsPerHour = TicksPerHour / TicksPerMillisecond;               //       3,600,000
        public const long MillisecondsPerDay = TicksPerDay / TicksPerMillisecond;                 //      86,400,000

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

@IDisposable let's apply the approved proposal #94545 (comment) and then we can track any further changes to bring it back to the design review.

@tarekgh tarekgh reopened this Jun 19, 2024
@IDisposable
Copy link
Contributor Author

New PR #103721

@tarekgh tarekgh added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Jun 19, 2024
@tarekgh tarekgh removed the in-pr There is an active PR which will close this issue when it is merged label Jun 20, 2024
@bartonjs
Copy link
Member

bartonjs commented Jun 25, 2024

Video

Based on the domain (the TimeSpan type itself), the belief is that long is correct for everything except HoursPerDay (int).

While that's not so nice for seconds * TimeSpan.MillisecondsPerSecond for passing to (e.g.) new CancellationTokenSource(int), it is more appropriate for working with the TimeSpan type itself.

namespace System.Collections.Generic;

public class TimeSpan: // elided details
{
        // more elision

        public const long MicrosecondsPerMillisecond = TicksPerMillisecond / TicksPerMicrosecond; //           1,000
        public const long MicrosecondsPerSecond = TicksPerSecond / TicksPerMicrosecond;           //       1,000,000
        public const long MicrosecondsPerMinute = TicksPerMinute / TicksPerMicrosecond;           //      60,000,000
        public const long MicrosecondsPerHour = TicksPerHour / TicksPerMicrosecond;               //   3,600,000,000
        public const long MicrosecondsPerDay = TicksPerDay / TicksPerMicrosecond;                 //  86,400,000,000

        public const long MillisecondsPerSecond = TicksPerSecond / TicksPerMillisecond;           //           1,000
        public const long MillisecondsPerMinute = TicksPerMinute / TicksPerMillisecond;           //          60,000
        public const long MillisecondsPerHour = TicksPerHour / TicksPerMillisecond;               //       3,600,000
        public const long MillisecondsPerDay = TicksPerDay / TicksPerMillisecond;                 //      86,400,000

        public const long SecondsPerMinute = TicksPerMinute / TicksPerSecond;                     //              60
        public const long SecondsPerHour = TicksPerHour / TicksPerSecond;                         //           3,600
        public const long SecondsPerDay = TicksPerDay / TicksPerSecond;                           //          86,400

        public const long MinutesPerHour = TicksPerHour / TicksPerMinute;                         //              60
        public const long MinutesPerDay = TicksPerDay / TicksPerMinute;                           //           1,440

        public const int HoursPerDay = TicksPerDay / TicksPerHour;                               //              24
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 25, 2024
@IDisposable
Copy link
Contributor Author

That makes perfect sense, let me know when/what the final should be and I will resubmit

@tannergooding
Copy link
Member

You should feel free to submit a PR matching it at any time now, we're in agreement as to the handling here now.

@tarekgh
Copy link
Member

tarekgh commented Jun 25, 2024

@IDisposable

let me know when/what the final should be and I will resubmit

It is ready now so you can resurrect or create a new PR now.

@IDisposable
Copy link
Contributor Author

Working on it... do we have a decision about the casting at call-site or modifying the parameters type as mentioned here?

We had settled on casting at the call site, last I remember... and suspect we should go that route now.

IDisposable added a commit to IDisposable/dotnet-runtime that referenced this issue Jun 25, 2024
Publicly expose the useful constants that `TimeSpan` had internally declared since they're rooted in hard reality and not changeable, and quite useful elsewhere. Everything is a `long` except `HoursPerDay` because that is useful in many non-TimeSpan contexts and rarely would overflow.

Added /// documentation for newly public constants and updated _System.Runtime.cs_ assembly API reference.

Use the newly exposed values in all the runtime places similar constants are declared in runtime **EXCEPT** did not use the `TimeSpan` constant in _EventLogInternal.cs_ since that source is also built for older frameworks, leave existing internal constant.

In _Calendar.cs_, use an `int` cast at the call-sites to `Add` method to prevent performance degradation, these calls are known not to overflow.

Fixes dotnet#94545
@dotnet-policy-service dotnet-policy-service bot added in-pr There is an active PR which will close this issue when it is merged labels Jun 25, 2024
@IDisposable
Copy link
Contributor Author

PR #103993 has hopefully final iteration :)

@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.DateTime in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
6 participants