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

Feature request: Add Environment.LongTickCount #28754

Closed
geoffkizer opened this issue Feb 21, 2019 · 9 comments
Closed

Feature request: Add Environment.LongTickCount #28754

geoffkizer opened this issue Feb 21, 2019 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@geoffkizer
Copy link
Contributor

Environment.TickCount rolls over, which makes it inconvenient to use. See e.g. dotnet/corefx#35401

To remedy this, we should add Environment.LongTickCount. This is trivial to implement on Windows using GetTickCount64.

public static class Environment
{
    public static long LongTickCount { get; } // or TickCount64
}
@stephentoub
Copy link
Member

@danmoseley
Copy link
Member

danmoseley commented Feb 22, 2019

There is precedent for adding 64 on properties on Process eg

        public int PeakWorkingSet { get { throw null; } }
        public long PeakWorkingSet64 { get { throw null; } }

also ContentLength64 on a couple networking classes. Altogether I see 11 public properties in CoreFX with the 64 suffix.
Conversely the only example of the Long prefix is

    public abstract partial class Array 
    {
        public int Length { get { throw null; } }
        public long LongLength { get { throw null; } }
   }

So it's worth considering

public static class Environment
{
    public static long TickCount64 { get; }
}

@stephentoub
Copy link
Member

Conversely the only example of the Long prefix is

There's also Enumerable.LongCount.

But I'd be fine with either the Long prefix or the 64 suffix.

@danmoseley
Copy link
Member

There's also Enumerable.LongCount.

Right I only looked at properties

@tannergooding
Copy link
Member

I think 64 or Int64 makes much more sense.

The framework guidelines also recommend not using language specific names (like Long), given that they can mean different things in different languages: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions#avoiding-language-specific-names

@jeroen-mostert
Copy link

Re: language-specific names, my favorite bug/gotcha in this regard is IDataReader.GetFloat() (which should have been IDataReader.GetSingle()). This has bitten numerous folks who thought to use .GetFloat() to read a T-SQL FLOAT, which is actually a double-precision type and has to be read with .GetDouble(). This is practically an unavoidable mistake if your language doesn't have a float type in the first place (e.g. VB.NET).

The only reason "long" is halfway acceptable in this regard is because it can be interpreted as a modifier rather than a type name (e.g. LongLength is Length, but "longer") and so is unlikely to be as harmful as the above.

@terrajobst
Copy link
Member

The most logical API proposal would be, which looks fine to me.

public static class Environment
{
    public static long TickCount64 { get; }
}

I'll bring it up in the next API review.

@terrajobst
Copy link
Member

terrajobst commented May 28, 2019

Video

And we landed on:

public static class Environment
{
    public static long TickCount64 { get; }
}

@danmoseley
Copy link
Member

Should we mark up for grabs or is someone planning to take this?

@stephentoub stephentoub self-assigned this May 30, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 14, 2020
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.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants