bpo-33178: Add BigEndianUnion, LittleEndianUnion classes to ctypes #25480
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/ctypes/test/test_byteswap.py
Outdated
| @@ -311,5 +311,68 @@ class S(Structure): | |||
| s2 = struct.pack(fmt, 0x12, 0x1234, 0x12345678, 3.14) | |||
| self.assertEqual(bin(s1), bin(s2)) | |||
|
|
|||
| def test_union_fields(self): | |||
There was a problem hiding this comment.
IMO this test would be better named something like test_union_fields_unsupported_byte_order.
Lib/ctypes/test/test_byteswap.py
Outdated
|
|
||
| # these fields do not support different byte order: | ||
| for typ in c_wchar, c_void_p, POINTER(c_int): | ||
| _fields_.append(("x", typ)) |
There was a problem hiding this comment.
The field appended here will not be removed and so will be available in the next iteration, which I assume is not what you want. I recommend using the + operator instead to avoid modifying the original list.
I would do something like this.
fields = [
("a", c_ubyte),
("b", c_byte),
("c", c_short),
("d", c_ushort),
("e", c_int),
("f", c_uint),
("g", c_long),
("h", c_ulong),
("i", c_longlong),
("k", c_ulonglong),
("l", c_float),
("m", c_double),
("n", c_char),
("b1", c_byte, 3),
("b2", c_byte, 3),
("b3", c_byte, 2),
("a", c_int * 3 * 3 * 3),
]
for typ in (c_wchar, c_void_p, POINTER(c_int)):
with self.assertRaises(TypeError):
class T(base):
_fields_ = fields + [("x", typ)]There was a problem hiding this comment.
Thanks for the review!
cpython/Lib/ctypes/test/test_byteswap.py
Lines 173 to 206 in 8d28e0a
test_struct_field_1 is nearly the same as the code you raised this comment on; will fix for the same issue.
|
The only improvement I could suggest at the moment is maybe using subtests so that we can easily distinguish what failed if the tests break. But I'll leave that to the person who will do the final review. https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests |
|
@iritkatriel since you have been active lately, would you mind having a look at this? No worries if you are not comfortable enough to review it. (let me know if you don't want me to ping you like this in the future) |
|
@FFY00 I don't mind the ping, but I also don't know much at all about ctypes. |
|
edit: @abalkin is listed as an expert on ctypes in the Python Developer's Guide, and as active on the Developer Log. |
|
@pablogsal any suggestions on a core developer to review this PR? |
|
@FFY00 @ringof @pablogsal What's the status of this PR? Projects such as pyembc could benefit from # ctypes currently does not implement the BigEndianUnion and LittleEndianUnion despite its documentation
# sais so. Therefore, we use simple Union for now. Details:
# https://stackoverflow.com/questions/49524952/bigendianunion-is-not-part-of-pythons-ctypes
# https://bugs.python.org/issue33178
if endian == "little":
_bases = {
_PyembcTarget.STRUCT: ctypes.LittleEndianStructure,
_PyembcTarget.UNION: ctypes.Union
}
elif endian == "big":
_bases = {
_PyembcTarget.STRUCT: ctypes.BigEndianStructure,
_PyembcTarget.UNION: ctypes.Union
}
else:
raise ValueError("Invalid endianness")I discovered this PR via https://bugs.python.org/issue33178#msg397291:
So there appears to be no activity on this since July 2021. (In a few days it'll be January 2022.) Is there any way I can help out? What would be the most productive thing I could do to help get this merged? Thanks for your time! |
|
@shawwn its awaiting core developer review - I wasn't able to find one with the experience and bandwidth to review this. I'm willing to do my part to respond to requests for changes. |
|
The following list of names is from recently closed PRs or PRs with a glancing involvement with ctypes. @iritkatriel @tiran @ericsnowcurrently @vstinner @pablogsal @markshannon @ambv @serhiy-storchaka I'm looking for a core review for this PR, but the Experts list does not suggest anybody who is recently active. Anybody have a suggestions who to reach out to do so? |
|
@gpshead All your comments are addressed. I'm not sure about the line length for what I did for the class definitions in the unsupported types tests - is there a general maximum and preferred line breaking style, I'll be glad to fix that. |
|
Thanks - glad it made it! |
https://bugs.python.org/issue33178
Adds the BigEndianUnion, LittleEndianUnion classes documented here: https://docs.python.org/3/library/ctypes.html#structure-union-alignment-and-byte-order but are unimplemented in ctypes.
Co-authored-by: syeberman (Sye van Der Veen)
Source for this PR is derived from a patch submitted by syeberman for bpo-19023:
https://bugs.python.org/issue19023
https://bugs.python.org/file31769/75843d82f6cf.diff
for which these changes were not included in the final patch used to close that issue.
https://bugs.python.org/issue33178