Skip to content

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

Merged
vstinner merged 3 commits intopython:masterfrom
ZackerySpytz:bpo-37170-PyLong_AsUnsignedLongLongMask-wrong-cast
Jun 6, 2019
Merged

bpo-37170: Fix the cast on error in PyLong_AsUnsignedLongLongMask()#13860
vstinner merged 3 commits intopython:masterfrom
ZackerySpytz:bpo-37170-PyLong_AsUnsignedLongLongMask-wrong-cast

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

ZackerySpytz commented Jun 6, 2019

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

mdickinson commented Jun 6, 2019

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

miss-islington commented Jun 6, 2019

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

miss-islington commented Jun 6, 2019

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

miss-islington commented Jun 6, 2019

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

miss-islington commented Jun 6, 2019

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

bedevere-bot commented Jun 7, 2019

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

@ZackerySpytz
Copy link
Contributor Author

ZackerySpytz commented Jun 7, 2019

@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

bedevere-bot commented Jun 7, 2019

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

bedevere-bot commented Jun 7, 2019

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

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

+2 more reviewers

@mdickinson mdickinson mdickinson approved these changes

@mangrisano mangrisano mangrisano approved these changes

Reviewers whose approvals may not affect merge requirements

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