Skip to content

bpo-35959: Rework integer overflow path in math.prod and add more tests #11809

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
Merged
pablogsal merged 1 commit into from
Mar 9, 2019
Merged

bpo-35959: Rework integer overflow path in math.prod and add more tests #11809

pablogsal merged 1 commit into from
Mar 9, 2019

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Feb 10, 2019 •
edited
Loading

The overflow check was relying on undefined behaviour as it was using the result of the multiplication to do the check, and once the overflow has already happened, any operation on the result is undefined behaviour.

I have moved the test to the correct class as well (TestMath).

https://bugs.python.org/issue35959

@pablogsal pablogsal requested a review from rhettinger February 10, 2019 20:42
@pablogsal pablogsal force-pushed the bpo35959 branch 7 times, most recently from 4a30482 to 5025c0d Compare February 10, 2019 21:46
@pablogsal pablogsal force-pushed the bpo35959 branch 2 times, most recently from 2e18189 to 933fb66 Compare February 10, 2019 21:56
@pablogsal pablogsal changed the title bpo-35959: Remove unneeded check in math.prod and add more tests bpo-35959: Rework integer overflow path in math.prod and add more tests Feb 10, 2019
mdickinson
mdickinson reviewed Feb 11, 2019
View reviewed changes
mdickinson
mdickinson reviewed Feb 11, 2019
View reviewed changes
@pablogsal
Copy link
Member Author

pablogsal commented Feb 11, 2019

@mdickinson Do you have some suggestions for extra checks to cover these corner cases?

eamanu
eamanu approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

LGTM. I am testing the PR on Unbuntu 18 04 x86_64 and run ok.

@pablogsal
Copy link
Member Author

pablogsal commented Feb 12, 2019 •
edited
Loading

@mdickinson At the end I have used Python2's approach for overflow handling. Could you review again this PR?

@mdickinson
Copy link
Member

mdickinson commented Feb 14, 2019

Could you review again this PR?

Will do, but I'm not going to have time before the weekend. If another core dev wants to review before then, that's fine with me.

@pablogsal pablogsal requested a review from mdickinson February 21, 2019 22:09
@pablogsal
Copy link
Member Author

pablogsal commented Mar 9, 2019

I am going to merge this one as this is a known problem and I do not want to miss more alphas. As the code used is the code of how Python2 managed overflow for multiplication and this is battle tested, this should be more than ok. In case we want to improve/speedup how we handle the overflow we can do it in another PR.

@pablogsal pablogsal merged commit 0411411 into python:master Mar 9, 2019
@pablogsal pablogsal deleted the bpo35959 branch March 9, 2019 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger

@mdickinson mdickinson Awaiting requested review from mdickinson

1 more reviewer

@eamanu eamanu eamanu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal @mdickinson @eamanu @the-knights-who-say-ni @bedevere-bot