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

bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMask() #13860

Merged
merged 3 commits into from
Jun 6, 2019
Merged

bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMask() #13860

merged 3 commits into from
Jun 6, 2019

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Jun 6, 2019 •
edited by bedevere-bot
Loading

vstinner
vstinner reviewed Jun 6, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

While we are on it, can you also update the doc?
https://docs.python.org/dev/c-api/long.html#c.PyLong_AsUnsignedLongLongMask

replace -1 with (unsigned long long) -1 in the doc.

Since it's a bug fix, can you also add a NEWS entry?

@ZackerySpytz
Copy link
Contributor Author

Thank you, @vstinner, for the review. I have made the requested changes.

3.6 only takes security fixes at this point, so I think the "needs backport to 3.6" label should be removed, no?

jdemeyer
jdemeyer requested changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@jdemeyer jdemeyer left a comment

Choose a reason for hiding this comment

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

Shouldn't there be a test added for this?

@vstinner
Copy link
Member

vstinner commented Jun 6, 2019

3.6 only takes security fixes at this point, so I think the "needs backport to 3.6" label should be removed, no?

Oops, I'm not used to Python 3.8 branch yet :-) I wanted to add "needs backport to 3.8" ;-) (fixed)

Shouldn't there be a test added for this?

Yes, a test would be good.

mdickinson
mdickinson approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM. Would it be feasible to write a regression test?

@mdickinson
Copy link
Member

Would it be feasible to write a regression test?

Ah, sorry; I should have read the other comments first.

@vstinner
Copy link
Member

vstinner commented Jun 6, 2019

Ah, sorry; I should have read the other comments first.

A test should be added.

mdickinson
mdickinson approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LEGTM! (Looks even gooder to me, now that it has a test.)

@vstinner vstinner merged commit dc24765 into python:master Jun 6, 2019
@miss-islington
Copy link
Contributor

Thanks @ZackerySpytz for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7, 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@miss-islington
Copy link
Contributor

Sorry, @ZackerySpytz and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker dc2476500d91082f0c907772c83a044bf49af279 3.7

@miss-islington
Copy link
Contributor

Sorry @ZackerySpytz and @vstinner, I had trouble checking out the 2.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker dc2476500d91082f0c907772c83a044bf49af279 2.7

@vstinner
Copy link
Member

vstinner commented Jun 6, 2019

@ZackerySpytz: oh, so backport failed. Would you mind to attempt to backport the change manually to 3.8 first? Once 3.8 fix will land, we can consider other backports.

ZackerySpytz added a commit to ZackerySpytz/cpython that referenced this pull request Jun 7, 2019
@ZackerySpytz
[3.8] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
276701e 
…sk() (pythonGH-13860)

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot
Copy link

GH-13891 is a backport of this pull request to the 3.8 branch.

@ZackerySpytz
Copy link
Contributor Author

@vstinner, I have created a backport to 3.8.

vstinner pushed a commit that referenced this pull request Jun 7, 2019
@ZackerySpytz @vstinner
[3.8] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
dd492d9 
…sk() (GH-13860) (GH-13891)

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@vstinner
Copy link
Member

vstinner commented Jun 7, 2019 •
edited
Loading

@vstinner, I have created a backport to 3.8.

Merged, thanks. Can you try to backport to 3.7 and 2.7? You might reuse the 3.8 commit or not, it's up to you.

ZackerySpytz added a commit to ZackerySpytz/cpython that referenced this pull request Jun 7, 2019
@ZackerySpytz
[3.7] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
f2365ec 
…sk() (pythonGH-13860).

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot
Copy link

GH-13896 is a backport of this pull request to the 3.7 branch.

vstinner pushed a commit that referenced this pull request Jun 7, 2019
@ZackerySpytz @vstinner
[3.7] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
e36ed47 
…sk() (GH-13860) (GH-13896)

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
ZackerySpytz added a commit to ZackerySpytz/cpython that referenced this pull request Jun 7, 2019
@ZackerySpytz
[2.7] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
c2442c4 
…sk() (pythonGH-13860).

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot
Copy link

GH-13898 is a backport of this pull request to the 2.7 branch.

vstinner pushed a commit that referenced this pull request Jun 7, 2019
@ZackerySpytz @vstinner
[2.7] bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMa…
2bfc2dc 
…sk() (GH-13860) (GH-13898)

(cherry picked from commit dc24765)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@ZackerySpytz @DinoV
bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMask() (p…
e1b2b4c 
…ythonGH-13860)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@jdemeyer jdemeyer jdemeyer requested changes

@mdickinson mdickinson mdickinson approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@mangrisano mangrisano mangrisano approved these changes

Assignees

@vstinner vstinner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ZackerySpytz @vstinner @mdickinson @miss-islington @bedevere-bot @jdemeyer @serhiy-storchaka @mangrisano @the-knights-who-say-ni