bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int

[pv]

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:

>>> import numpy, ctypes >>> numpy.asarray(memoryview(ctypes.c_long(42))) /usr/lib64/python3.5/site-packages/numpy/core/numeric.py:482: RuntimeWarning: Item size computed from the PEP 3118 buffer format string does not match the actual item size. return array(a, dtype, copy=False, order=order) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib64/python3.5/site-packages/numpy/core/numeric.py", line 482, in asarray return array(a, dtype, copy=False, order=order) ValueError: setting an array element with a sequence. 

• struct format string syntax
• bpo-10746

https://bugs.python.org/issue10746

[the-knights-who-say-ni]

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 the PSF 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:

1. Sign the PSF contributor agreement
2. Wait at least a day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
3. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

[codecov]

Codecov Report

Merging #31 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

@@ 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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ffb99...b4cf54b. Read the comment docs.

[pv]

CLA signed.

[pitrou]

This function should be declared static.

[pitrou]

I don't understand this line. Can you show an example where tp._type_ is necessary?

[pv]

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.

[pv]

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 + <i 

Added explanatory comment in the test.

[pitrou]

It would be nice to add a non-empty error message, for example "SIZEOF_INT is not one of the expected values".

[pitrou]

Ditto.

[pitrou]

Ditto.

[pv]

Sorry for the delay, updated the PR based on comments.

[pv]

ping?

[pitrou]

Sorry for the delay.

[pitrou]

I still find this difficult to read and understand. Can't you rewrite the unit tests and the native_types array to get rid of the double indirection?

[pitrou]

Internal ctypes type codes are not even documented, are they?

[pitrou]

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?).

[pv]

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 when multiple equivalent possibilities exist, is platform and possibly compiler dependent. If you are suggesting to write all possibilities explicitly, you have to write different tables for win32, win64, linux32, etc. Can you specify more precisely what you object to: - using replace in strings to produce appropriate codes rather than writing them explicitly, - not hardcoding codes for equivalent integer types, - something else? 26.8.2017 11.04 "Antoine Pitrou" <[email protected]> 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> .

[pv]

The internal types codes are not documented. But do you have better suggestions how to write the strings? They cannot be written in PEP3118 format because the produced strings are platform etc dependent. 26.8.2017 11.04 "Antoine Pitrou" <[email protected]> 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> .

[pv]

This of course was ignored previously in the tests, because the ctypes implementation of this stuff was just wrong. 26.8.2017 11.37 "Pauli Virtanen" <[email protected]> kirjoitti: The internal types codes are not documented. But do you have better suggestions how to write the strings? They cannot be written in PEP3118 format because the produced strings are platform etc dependent. 26.8.2017 11.04 "Antoine Pitrou" <[email protected]> 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> .

[pitrou]

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.

[pv]

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" <[email protected]> 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> .

[pv]

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" <[email protected]> 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" <[email protected]> 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> .

[pitrou]

You are already encoding platform-dependent information (the size mappings) in code_map? Why don't you fold that into native_types?

[pv]

native_types and the other tables contain also compound types where these type codes appear. To me the present approach appeared the simplest solution without code repetition. 26.8.2017 20.13 "Pauli Virtanen" <[email protected]> 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> > . >

[pv]

Ok, once more then, without magic. Is this what you wanted?

[pitrou]

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 c_int aliases c_long, for example (which is a weird oddity, but I guess it's not this PR's responsability to change that).

One additional thing: can you add a NEWS entry? We nowadays use the blurb CLI tool for that.