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

Fix int overflow on 32-bit architectures #5252

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Fix int overflow on 32-bit architectures #5252

merged 1 commit into from
Feb 6, 2023

Conversation

Arnie97
Copy link
Contributor

@Arnie97 Arnie97 commented Jan 28, 2023

$  GOARCH=386 GOOS=linux go build ./...
# github.com/dolthub/dolt/go/store/skip
store/skip/list.go:152:21: maxCount (untyped int constant 4294967294) overflows int

See also dolthub/vitess#216

@timsehn
Copy link
Sponsor Contributor

timsehn commented Jan 28, 2023

Thanks @reltuk will look at this one.

@@ -21,7 +21,7 @@ import (

const (
maxHeight = 9
maxCount = math.MaxUint32 - 1
maxCount = math.MaxInt - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be better as:

Suggested change
maxCount = math.MaxInt - 1
maxCount = math.MaxInt32 - 1

for now. The structure will not work with a number of entries > MaxUint32, so we don't want to set it to MaxInt - 1 on a 64-bit platform.

For now we can give up the two billion possible extra entries on 64-bit in order to get this working on 32-bit.

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the report! Generally looks great to me. One suggested change.

@reltuk reltuk merged commit 8dc1d82 into dolthub:main Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants