Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int#31
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedFeb 11, 2017
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:
Thanks again to your contribution and we look forward to looking at it! |
Codecov Report
@@ Coverage Diff @@## master #31 +/- ##==========================================+ Coverage 82.37% 82.37% +<.01%========================================== Files 1427 1427 Lines 350948 350957 +9 ==========================================+ Hits 289088 289114 +26+ Misses 61860 61843 -17 Continue to review full report at Codecov.
|
CLA signed. |
Modules/_ctypes/_ctypes.c Outdated
| since the endianness may need to be swapped to a non-native one | ||
| later on. | ||
| */ | ||
| char * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This function should be declaredstatic.
Lib/ctypes/test/test_pep3118.py Outdated
| # Since some types may be aliases to each other, look up the | ||
| # struct code based on the actual ctypes type | ||
| tp = code_map[ctypes_code][0] | ||
| struct_code = code_map[tp._type_][1][sizeof(tp)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand this line. Can you show an example wheretp._type_ is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I must say I don't understand it either anymore. It may be leftover from refactoring.
Since the tests seem to pass, I now simplified it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah no, it was due to a int/long failure on windows:
======================================================================FAIL: test_native_types (ctypes.test.test_pep3118.Test)----------------------------------------------------------------------Traceback (most recent call last): File "C:\projects\cpython\lib\ctypes\test\test_pep3118.py", line 56, in test_native_types normalize(replace_type_codes(fmt)))AssertionError: '<l' != '<i'- <l+ <iAdded explanatory comment in the test.
Modules/_ctypes/_ctypes.c Outdated
| case 'i': pep_code = 'q'; break; | ||
| case 'I': pep_code = 'Q'; break; | ||
| #else | ||
| # error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would be nice to add a non-empty error message, for example "SIZEOF_INT is not one of the expected values".
Modules/_ctypes/_ctypes.c Outdated
| case 'l': pep_code = 'q'; break; | ||
| case 'L': pep_code = 'Q'; break; | ||
| #else | ||
| # error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto.
Modules/_ctypes/_ctypes.c Outdated
| #elif SIZEOF__BOOL == 8 | ||
| case '?': pep_code = 'Q'; break; | ||
| #else | ||
| # error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto.
Sorry for the delay, updated the PR based on comments. |
d43d02d toeed2ee1Compareping? |
Sorry for the delay. |
Lib/ctypes/test/test_pep3118.py Outdated
| # | ||
| # Example: on Windows c_long translates to '<i', not '<l'. | ||
| tp = code_map[ctypes_code][0] | ||
| struct_code = code_map[tp._type_][1][sizeof(tp)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I still find this difficult to read and understand. Can't you rewrite the unit tests and thenative_types array to get rid of the double indirection?
Lib/ctypes/test/test_pep3118.py Outdated
| @@ -111,7 +146,11 @@ class Complete(Structure): | |||
| # | |||
| # This table contains format strings as they look on little endian | |||
| # machines. The test replaces '<' with '>' on big endian machines. | |||
| # The type codes written below are the internal ctypes type codes, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Internal ctypes type codes are not even documented, are they?
Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). |
A static translation table does not exist, because the size of the types,and also which type code is produced for some of the integer types whenmultiple equivalent possibilities exist, is platform and possibly compilerdependent. If you are suggesting to write all possibilities explicitly, youhave to write different tables for win32, win64, linux32, etc. Can youspecify more precisely what you object to:- using replace in strings to produce appropriate codes rather than writingthem explicitly,- not hardcoding codes for equivalent integer types,- something else?26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti: … Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> . |
The internal types codes are not documented. But do you have bettersuggestions how to write the strings? They cannot be written in PEP3118format because the produced strings are platform etc dependent.26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti: … Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> . |
This of course was ignored previously in the tests, because the ctypesimplementation of this stuff was just wrong.26.8.2017 11.37 "Pauli Virtanen" <pav@iki.fi> kirjoitti:The internal types codes are not documented. But do you have bettersuggestions how to write the strings? They cannot be written in PEP3118format because the produced strings are platform etc dependent.26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti: … Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> . |
My main gripe here is that you shouldn't need Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them. |
The expected pep strings are platform-dependent. Do you agree with thatstatement? Do you mean the entries in the native_types table should betranslate_typecodes("...") calls in the table itself. Or, do you meancode_map should be replaced by a slew of if statemebts encoding theplatform dependence?26.8.2017 19.49 "Antoine Pitrou" <notifications@github.com> kirjoitti: … My main gripe here is that you shouldn't need code_map at all, let alone the complicated double indirection inside that table. Just add/replace whatever information is missing to native_types. Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci> . |
I think I need railroad track now :)Could you give an example how you would like the test for the pep3118format string for c_long to look like?26.8.2017 20.08 "Pauli Virtanen" <pav@iki.fi> kirjoitti:The expected pep strings are platform-dependent. Do you agree with thatstatement? Do you mean the entries in the native_types table should betranslate_typecodes("...") calls in the table itself. Or, do you meancode_map should be replaced by a slew of if statemebts encoding theplatform dependence?26.8.2017 19.49 "Antoine Pitrou" <notifications@github.com> kirjoitti: … My main gripe here is that you shouldn't need code_map at all, let alone the complicated double indirection inside that table. Just add/replace whatever information is missing to native_types. Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci> . |
You are already encoding platform-dependent information (the size mappings) in |
pv commentedAug 26, 2017 via email• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
native_types and the other tables contain also compound types where thesetype codes appear. To me the present approach appeared the simplest solutionwithout code repetition.26.8.2017 20.13 "Pauli Virtanen" <pav@iki.fi> kirjoitti: … I think I need railroad track now :) Could you give an example how you would like the test for the pep3118 format string for c_long to look like? 26.8.2017 20.08 "Pauli Virtanen" ***@***.***> kirjoitti: The expected pep strings are platform-dependent. Do you agree with that statement? Do you mean the entries in the native_types table should be translate_typecodes("...") calls in the table itself. Or, do you mean code_map should be replaced by a slew of if statemebts encoding the platform dependence? 26.8.2017 19.49 "Antoine Pitrou" ***@***.***> kirjoitti:> My main gripe here is that you shouldn't need code_map at all, let alone> the complicated double indirection inside that table. Just add/replace> whatever information is missing to native_types.>> Also we don't care at all about ctypes internal code types, so the tests> would be clearer if they didn't involve them.>> —> You are receiving this because you authored the thread.> Reply to this email directly, view it on GitHub> <#31 (comment)>, or mute> the thread> <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci>> .> |
Ok, once more then, without magic. Is this what you wanted? |
Thank you! :-) I find this much better. In particular, it's explicit that the exact type code does not depend only on the type size but also on whether One additional thing: can you add a NEWS entry? We nowadays use theblurb CLI tool for that. |
See the pushed commit. It's parent isa30f6d4, which is the current pythonmaster.28.8.2017 14.57 "Antoine Pitrou" <notifications@github.com> kirjoitti: … Hmm... are you sure you updated master before rebasing? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5tR7nhHuhdygAJlPE6t0pEI26cNrks5scqsxgaJpZM4L-Rci> . |
I see. Well, it passed on AppVeyor anyway, so I'm going to merge. Thanks for persisting in this! |
…_int (pythonGH-31)Ctypes currently produces wrong pep3118 type codes for several types.E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,but it should be "<q" instead for sizeof(c_long) == 8The problem is that the '<>' endian specification in the struct syntaxalso turns on the "standard size" mode, which makes type characters havea platform-independent meaning, which does not match with the codes usedinternally in ctypes. The struct module format syntax also does notallow specifying native-size non-native-endian items.This commit adds a converter function that maps the internal ctypescodes to appropriate struct module standard-size codes in the pep3118format strings. The tests are modified to check for this.(cherry picked from commit07f1658)
…_int (pythonGH-31)Ctypes currently produces wrong pep3118 type codes for several types.E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,but it should be "<q" instead for sizeof(c_long) == 8The problem is that the '<>' endian specification in the struct syntaxalso turns on the "standard size" mode, which makes type characters havea platform-independent meaning, which does not match with the codes usedinternally in ctypes. The struct module format syntax also does notallow specifying native-size non-native-endian items.This commit adds a converter function that maps the internal ctypescodes to appropriate struct module standard-size codes in the pep3118format strings. The tests are modified to check for this..(cherry picked from commit07f1658)
…_int (GH-31) (#3241)Ctypes currently produces wrong pep3118 type codes for several types.E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,but it should be "<q" instead for sizeof(c_long) == 8The problem is that the '<>' endian specification in the struct syntaxalso turns on the "standard size" mode, which makes type characters havea platform-independent meaning, which does not match with the codes usedinternally in ctypes. The struct module format syntax also does notallow specifying native-size non-native-endian items.This commit adds a converter function that maps the internal ctypescodes to appropriate struct module standard-size codes in the pep3118format strings. The tests are modified to check for this.(cherry picked from commit07f1658)
…_int (GH-31) (#3242)[2.7]bpo-10746: Fix ctypes PEP 3118 type codes for c_long, c_bool, c_int (GH-31)Ctypes currently produces wrong pep3118 type codes for several types.E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,but it should be "<q" instead for sizeof(c_long) == 8The problem is that the '<>' endian specification in the struct syntaxalso turns on the "standard size" mode, which makes type characters havea platform-independent meaning, which does not match with the codes usedinternally in ctypes. The struct module format syntax also does notallow specifying native-size non-native-endian items.This commit adds a converter function that maps the internal ctypescodes to appropriate struct module standard-size codes in the pep3118format strings. The tests are modified to check for this..(cherry picked from commit07f1658)
…ython#31)Ctypes currently produces wrong pep3118 type codes for several types.E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,but it should be "<q" instead for sizeof(c_long) == 8The problem is that the '<>' endian specification in the struct syntaxalso turns on the "standard size" mode, which makes type characters havea platform-independent meaning, which does not match with the codes usedinternally in ctypes. The struct module format syntax also does notallow specifying native-size non-native-endian items.This commit adds a converter function that maps the internal ctypescodes to appropriate struct module standard-size codes in the pep3118format strings. The tests are modified to check for this.
There were cases where data["commit"]["committer"] is null.
Uh oh!
There was an error while loading.Please reload this page.
Ctypes currently produces wrong pep3118 type codes for several types.
E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,
but it should be "<q" instead for sizeof(c_long) == 8
The problem is that the '<>' endian specification in the struct syntax
also turns on the "standard size" mode, which makes type characters have
a platform-independent meaning, which does not match with the codes used
internally in ctypes. The struct module format syntax also does not
allow specifying native-size non-native-endian items.
This commit adds a converter function that maps the internal ctypes
codes to appropriate struct module standard-size codes in the pep3118
format strings. The tests are modified to check for this.
Example of the current problem in practice:
https://bugs.python.org/issue10746