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.

Sign up for GitHub

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

Jump to bottom

Fix integer overflow error when building for 386 #1541

Merged
merged 6 commits into from
Oct 2, 2020
Merged

Fix integer overflow error when building for 386 #1541

merged 6 commits into from
Oct 2, 2020

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Sep 24, 2020 •
edited by manishrjain
Loading

The untyped const causes conversion to int, for which the value is too large.
Use a typed const instead that is large enough to store value.

This change is Reviewable

@gammazero
Fix integer overflow error when building for 386
fc3c766 
The untyped const causes conversion to int, for which the value is too large.  Use a typed const instead that is large enough to store value.
@CLAassistant
Copy link

CLAassistant commented Sep 24, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

damz
damz suggested changes Sep 24, 2020
View reviewed changes
badger/cmd/backup.go Outdated Show resolved Hide resolved
badger/cmd/restore.go Outdated Show resolved Hide resolved
value.go Outdated Show resolved Hide resolved
jarifibrahim
jarifibrahim approved these changes Sep 27, 2020
View reviewed changes
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Thanks @gammazero 🎉 . This might fix the build but we've seen other cannot allocate memory errors on 386. We can handle those in a separate PR.

@NamanJain8 review please.

value.go Outdated Show resolved Hide resolved
@jarifibrahim
Copy link
Contributor

Thanks for helping with the review @damz :)

NamanJain8
NamanJain8 approved these changes Sep 27, 2020
View reviewed changes
Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks, @gammazero.

@NamanJain8
Copy link
Contributor

Not directly related to the PR itself, @jarifibrahim I was wondering about the use of int64 for ValueLogFileSize and uint32 for maxVlogFileSize. Maybe we need to revisit the types assigned to these.

@gammazero @jarifibrahim
Update value.go
8163afd 
Co-authored-by: Ibrahim Jarif <[email protected]>
@gammazero
Copy link
Contributor Author

The DeepSource and the travis-ci build failures are not related to this PR, and can/should be fixed in a separate PR.

@jarifibrahim jarifibrahim merged commit 68fb85d into hypermodeinc:master Oct 2, 2020
@jarifibrahim
Copy link
Contributor

Thanks for fixing this @gammazero

jarifibrahim pushed a commit that referenced this pull request Oct 2, 2020
@gammazero
Fix integer overflow error when building for 386 (#1541)
8239f9b 
The untyped const causes conversion to int, for which the value is too large.
Use a typed const instead that is large enough to store value.
@gammazero gammazero deleted the build386 branch October 4, 2020 18:27
@jsoriano
Copy link

jsoriano commented Oct 7, 2020

Hi, will this fix be backported to v2.2007?

This was referenced Oct 7, 2020
Merged
Merged
Merged
Merged
@jsoriano
Copy link

jsoriano commented Oct 7, 2020 •
edited
Loading

Hi, will this fix be backported to v2.2007?

I have created #1558 to fix this issue in v2.2007.

jarifibrahim pushed a commit that referenced this pull request Oct 12, 2020
@jsoriano
Fix integer overflow on 32-bit architectures (#1558)
f5a7e0a 
The untyped const causes conversion to int, for which the value is too large.
Use a typed const instead that is large enough to store value.

Similar to #1541, but for v2.2007 branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damz damz damz requested changes

@jarifibrahim jarifibrahim jarifibrahim approved these changes

@NamanJain8 NamanJain8 NamanJain8 approved these changes

@ashish-goswami ashish-goswami Awaiting requested review from ashish-goswami

@manishrjain manishrjain Awaiting requested review from manishrjain

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gammazero @CLAassistant @jarifibrahim @NamanJain8 @jsoriano @damz