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

math: add MaxUint, MinInt, MaxInt #28538

Open
Silentd00m opened this issue Nov 1, 2018 · 12 comments
Open

math: add MaxUint, MinInt, MaxInt #28538

Silentd00m opened this issue Nov 1, 2018 · 12 comments

Comments

@Silentd00m
Copy link

@Silentd00m Silentd00m commented Nov 1, 2018

The math package is lacking MaxInt, MaxUint, MinInt and MinUint.

This makes writing programs that assign a max-value to int or uint without manually specifying the value impossible, if you want to run the program on both 32 and 64 bit platforms.

I have written an implementation of the constants mentioned above here: #28537

@gopherbot gopherbot added this to the Proposal milestone Nov 1, 2018
@gopherbot gopherbot added the Proposal label Nov 1, 2018
@bontibon
Copy link
Contributor

@bontibon bontibon commented Nov 2, 2018

Isn't this platform dependent, not independent?

@Silentd00m
Copy link
Author

@Silentd00m Silentd00m commented Nov 2, 2018

It will give the correct max and min values for int and uint on 32 and 64 bit platforms.

I don't have access to 8 and 16 bit ones but it should work there as well due to how it's calculated.

@deanveloper
Copy link

@deanveloper deanveloper commented Nov 5, 2018

In your specific implementation, you have them as typed constants instead of untyped. That's probably not preferable as all other constants defined for limits are untyped constants.

implementation using unsafe: https://play.golang.org/p/VP6emWcsFSF
implementation without unsafe: https://play.golang.org/p/1thwBR0fWr3 (The name of wordSize is incorrect, as the int type does not necessarily correspond with the word size in the given environment)

@deanveloper
Copy link

@deanveloper deanveloper commented Nov 5, 2018

Also, while you're at it, might be a good idea to add a max uintptr as well.

Pretty sure the following is the only way to do it that results in an untyped constant:

const MinUintptr = 0
const MaxUintptr = 1<<(unsafe.Sizeof(uintptr(0))*8 - 1)<<1 - 1

Also, probably a good idea to get rid of MinUint and MinUintptr. Should just use 0. Go doesn't supply MinUint8 or MinUint16 etc.

@deanveloper
Copy link

@deanveloper deanveloper commented Nov 6, 2018

I realized there may be a licensing issue with my implementation that doesn't use unsafe. I got the code to retrieve the number of bits that an int holds (32 << (^uint(0) >> 63)) from go101, I thought the license was open, my bad. The license is available here.

The implementation that does use unsafe and the uintptr however, I made myself and is all good to use.

cc @go101

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 6, 2018

No need to worry about licensing issues for such a short purely-functional expression.

@Silentd00m
Copy link
Author

@Silentd00m Silentd00m commented Nov 6, 2018

I played around a bit with the changes @deanveloper proposed, especially the removal of the typing in the constants.

Having the constants typed has actually one big advantage: The constant won't overflow due to incorrect type detection/resolution.

https://play.golang.org/p/_iFMKqyauP9

When it is typed it should also protect against type narrowing.

@deanveloper
Copy link

@deanveloper deanveloper commented Nov 6, 2018

Having the constants typed has actually one big advantage: The constant won't overflow due to incorrect type detection/resolution.

This is correct. Pretty sure there is an issue to make the math constants typed, although I can't immediately find it. But a big advantage to having them untyped is you can do things such as the following:

dur := time.Hour * 100000000000
// can we fit dur in an int?
canFit := dur < math.MaxInt && dur > math.MinInt

or another example

func Longest(durs ...time.Duration) time.Duration {
    longestDir := math.MinInt64
    // compare each dur to longestDur and update
}

Although arguably it would be better to need to manually convert the constants to int and int64 as you need them. There's good arguments on both sides

@mvdan
Copy link
Member

@mvdan mvdan commented Nov 26, 2018

I always thought these were already part of the math package. Unless there's a good reason not to include them, I think consistency is a good enough argument to add them.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 28, 2018

We already have math.MaxInt32 etc for all the sized types. Adding the unsized ones seems fine. It's a little weird that they're not there. @griesemer points out that in math/bits we have a bunch of sized functions but decided to include unsized ones too. Sure, we can add Int and Uint variants.

Note that there is no math.MinUint8 etc so there should be no math.MinUint (it's always zero).

I'd want to see a compelling reason for MaxUintptr though. Let's leave that one out.

@rsc rsc modified the milestones: Proposal, Go1.13 Nov 28, 2018
@rsc rsc changed the title proposal: math: Add platform independent Max- and Min- for int and uint math: add MaxUint, MinInt, MaxInt Nov 28, 2018
@agnivade agnivade modified the milestones: Go1.13, Go1.14 May 25, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@rverma-jm
Copy link

@rverma-jm rverma-jm commented Dec 4, 2019

can add math.AbsInt()/AvgInt() as well, even better a whole wrapper as equivalent to float type. We found usually ints are more used than floats. Appreciate a varargs implementation of min/max/avg as well :)

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 4, 2019

@rverma-jm This proposal is specifically about the functions mentioned in the title. Please do not hijack this proposal as it is now accepted. Instead open a new proposal if you want to add something else. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.