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 #33

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Fix int overflow #33

merged 4 commits into from
Jun 17, 2021

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jun 17, 2021

Closes #27

@cpuguy83 cpuguy83 force-pushed the fix_int_overflow branch 3 times, most recently from 8a69844 to 7879a6d Compare June 17, 2021 22:10
@cpuguy83
Copy link
Member Author

Test run here: https://github.com/cpuguy83/go-ansiterm/pull/5/checks?check_run_id=2853454830

Apparently there's another bug as well which should be fixed by #27

@cpuguy83
Copy link
Member Author

@jstarks @kevpar

@cpuguy83 cpuguy83 force-pushed the fix_int_overflow branch 2 times, most recently from 69bcbca to d70676d Compare June 17, 2021 22:23
@cpuguy83 cpuguy83 force-pushed the fix_int_overflow branch 3 times, most recently from d227edb to cce61a1 Compare June 17, 2021 22:35
eclipseo and others added 4 commits June 17, 2021 22:36
// syscall uses negative numbers
// windows package uses very big uint32
// Keep these switches split so we don't have to convert ints too much.
switch uint32(nFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I went over a few variations as well; would casting to int64 work? docker/cli@97da45d

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but I figured it would be better to split since these are really different types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think ultimately we should drop the syscall one (and introduce a breaking change once)

Copy link
Contributor

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@jstarks
Copy link
Collaborator

jstarks commented Jun 17, 2021

This time we got it for sure.

@jstarks jstarks merged commit d185dfc into Azure:master Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants