gh-60580: Fix a wrong type of ctypes.wintypes.BYTE#97579
gh-60580: Fix a wrong type of ctypes.wintypes.BYTE#97579abalkin merged 8 commits intopython:mainfrom
ctypes.wintypes.BYTE#97579Conversation
Misc/NEWS.d/next/Library/2022-09-26-21-18-47.gh-issue-60580.0hBgde.rst
Outdated
Show resolved
Hide resolved
|
Does a trivial change like this (addition of a single character) warrant the CLA? |
|
@techtonik Would you mind to sign the CLA to get your PR acepted please? See a message of a bot above for details. |
Unfortunately it is impossible for a core dev to merge a PR without the CLA being signed, as the CLA check is a "required" status check. |
|
What would you recommend to do? The change is way too trivial to give a CLA-covered alternative. Also it seems impossible to call a user who never contributed to |
Squash the PR so that GitHub no longer recognises the first commit as being authored by @techtonik (thus evading the CLA bot), but make sure to give them credit in the commit message and PR description. Since this is, as you say, a small, relatively trivial PR, I think that should be fine. |
d6adc26 to
cf70637
Compare
|
@abalkin would you mind merging this PR as a ctypes expert? |
|
I am surprised that this changes does not require a change to any tests. This means that we don't have enough tests, but potentially this change can lead to some subtle change in behavior. Can someone provide a use case where this change makes a difference? |
The only case I've found inside CPython is cpython/Lib/ctypes/__init__.py Lines 224 to 225 in dfad678 cpython/Lib/ctypes/__init__.py Lines 231 to 232 in dfad678 Lines 808 to 811 in dfad678 However, |
|
It seems that external shim generation tools that might rely on the layout string will stay compatible. |
|
Curiously, the same question was asked when this issue was first raised:
A test was then suggested that should be sensitive to the change. We need to add a unit test before this change can be merged. |
|
We can just add a simple signedness test. Before the patch: >>> ctypes.wintypes.BYTE(200).value < 128
Trueafter the patch: >>> ctypes.wintypes.BYTE(200).value < 128
FalseSince |
|
@Yhg1s - please advise if this bug fix is appropriate for 3.12 and what documentation it will require in "What’s New In Python"? TL;DR: This PR changes |
Yes, it's appropriate for 3.12. (We're not in beta phase yet, so any fix appropriate for any release is appropriate for 3.12.) I don't think it's worth mentioning in What's New, since it's just a minor bugfix. |
Created from a patch file attached to an issue, by Anatoly Techtonik.
c51e475 to
057c925
Compare
|
Hmm, I wonder how anyone (including me - as I used wintypes.BYTE a lot in the past) missed this. I was just about to open a PR, as I'm working with [Ms.Learn]: SYSTEM_POWER_STATUS structure (winbase.h). Regarding signed / unsigned types: [SO]: Maximum and minimum value of C types integers from Python (@CristiFati's answer). |
This PR is created from a .diff file attached to the issue, by Anatoly Techtonik (@techtonik on GitHub).