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 overflow bug in prevpow #43410

Merged
merged 9 commits into from
Dec 20, 2021
Merged

Conversation

oscardssmith
Copy link
Member

Previously prevpow(10, 1234567890123456789) gave -8446744073709551616 due to integer overflow.

@oscardssmith oscardssmith added maths Mathematical functions bugfix This change fixes an existing bug labels Dec 12, 2021
@vchuravy
Copy link
Member

Needs a test

base/intfuncs.jl Outdated Show resolved Hide resolved
base/intfuncs.jl Outdated Show resolved Hide resolved
base/intfuncs.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

I think this is still not fully correct since x isn't necessarily the same type as a, so x could be greater than typemax(a) which means that p*a overflowing isn't necessarily sufficient here. So perhaps we should still promote a and x first?

@oscardssmith
Copy link
Member Author

That would be a somewhat breaking change since it would change the return type.

@oscardssmith
Copy link
Member Author

TLDR is I think that the correct behavior here is either this or to throw an error, which we can't do easily (since there is no Base.checked_pow). As such, I think this is what we should merge since it is a strict improvement.

@oscardssmith
Copy link
Member Author

@simeonschaub do you think this is mergable as is?

@KristofferC
Copy link
Member

Might a well do a pkgeval?

@KristofferC
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master")

@oscardssmith
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member

Looks ok.

@oscardssmith
Copy link
Member Author

Merging monday sans objections

@oscardssmith oscardssmith merged commit f835c24 into master Dec 20, 2021
@oscardssmith oscardssmith deleted the oscardssmith-fix-prevpow-overflow branch December 20, 2021 16:26
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants