GeezerVison

All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

It is an interesting idea.

But what should be the size and the alignment of B in the following example?

class A(Structure):
    x: c_uint32

class B(A):
    _align_ = 8
    y: c_uint32

Thanks for the review!

My intuition would say "8 and 8".
Running the code shows that while the size is 8, the alignment of B is 4, not 8 as specified.
To sanity check I wrote a c++ equivalent of the above and it has size 8, alignment 8.
I'll work on fixing this issue and add some unit tests to cover these cases also, thanks!
I'll also add the news snippet which I have been meaning to do...

!buildbot s390x

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 2329fa3 🤖

The command will test the builders whose names match following regular expression: s390x

The builders matched are:

• s390x RHEL7 LTO + PGO PR
• s390x Fedora LTO PR
• s390x RHEL7 LTO PR
• s390x SLES PR
• s390x Fedora Rawhide LTO PR
• s390x Fedora Clang Installed PR
• s390x RHEL8 Refleaks PR
• s390x Fedora Rawhide LTO + PGO PR
• s390x RHEL8 LTO + PGO PR
• s390x RHEL8 PR
• s390x Fedora Rawhide Refleaks PR
• s390x Fedora Refleaks PR
• s390x Fedora Clang PR
• s390x Fedora LTO + PGO PR
• s390x Fedora Rawhide PR
• s390x Fedora PR
• s390x RHEL8 LTO PR
• s390x Fedora Rawhide Clang PR
• s390x Debian PR
• s390x RHEL7 Refleaks PR
• s390x RHEL7 PR
• s390x Fedora Rawhide Clang Installed PR

https://buildbot.python.org/all/#/builders/390/builds/1350/steps/5/logs/stdio

======================================================================
FAIL: test_aligned_string (test.test_ctypes.test_aligned_structures.TestAlignedStructures.test_aligned_string)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/pull_request.edelsohn-debian-z/build/Lib/test/test_ctypes/test_aligned_structures.py", line 26, in test_aligned_string
    self.assertEqual(main.first, 7)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
AssertionError: 117440512 != 7
======================================================================
FAIL: test_aligned_struct_in_union (test.test_ctypes.test_aligned_structures.TestAlignedStructures.test_aligned_struct_in_union)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/pull_request.edelsohn-debian-z/build/Lib/test/test_ctypes/test_aligned_structures.py", line 158, in test_aligned_struct_in_union
    self.assertEqual(main.first, 1)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
AssertionError: 16777216 != 1
======================================================================
FAIL: test_aligned_structures (test.test_ctypes.test_aligned_structures.TestAlignedStructures.test_aligned_structures)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/pull_request.edelsohn-debian-z/build/Lib/test/test_ctypes/test_aligned_structures.py", line 64, in test_aligned_structures
    self.assertEqual(main.y, 7)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^
AssertionError: 117440512 != 7
======================================================================
FAIL: test_aligned_subclasses (test.test_ctypes.test_aligned_structures.TestAlignedStructures.test_aligned_subclasses)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/pull_request.edelsohn-debian-z/build/Lib/test/test_ctypes/test_aligned_structures.py", line 106, in test_aligned_subclasses
    self.assertEqual(main.a, 1)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^
AssertionError: 16777216 != 1
======================================================================
FAIL: test_aligned_union (test.test_ctypes.test_aligned_structures.TestAlignedStructures.test_aligned_union)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/pull_request.edelsohn-debian-z/build/Lib/test/test_ctypes/test_aligned_structures.py", line 129, in test_aligned_union
    self.assertEqual(main.first, 1)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
AssertionError: 16777216 != 1
----------------------------------------------------------------------

In the process of making the tests run with both big and little endian versions of the Structure and Union classes I found something curious...

from ctypes import *

class AlignedUnion(BigEndianUnion):
    _fields_ = [
        ("a", c_uint32),
        ("b", c_int32),
    ]

class Main(BigEndianStructure):
    _fields_ = [
        ("first", c_uint32),
        ("union", AlignedUnion),
    ]

m = Main()

If I run this on my machine (little endian), I get the following error:
TypeError: This type does not support other endian: <class '__main__.AlignedUnion'>

This seems like a bug and is the reason the last few tests look the way they do...

It was fixed not long time ago in #105102.

It was fixed not long time ago in #105102.

Ok great!

I know that as per the requirements for PR's it's required to make each commit separate and to not rebase, but is it fine to rebase the whole branch onto the current head so that I get the fix for this? I haven't contributed before and just want to make sure I'm following the right procedure...

Edit: Oh I see you already merged the code into the branch, thanks!

Don't rebase, just merge. I just merged main into your branch, you have to pull the changes.

Thanks for the thorough review!
I believe I have addressed all the comments and added some further tests to test the case of a subclassed Union where the subclass has an alignment.
I used struct.pack in most places to clean it except for one where the format string was just too ugly looking to make it obvious what the data would be at a glance.

I realised that I have neither documented nor tested the case of having both _align_ and _pack_ attributes. I'll add that also as I think it's important to understand how it should behave.

Sorry for the ping @serhiy-storchaka just checking to see if there was anything else I needed to address with this.
I believe I have covered the points you raised previously as well as the typo raised in the documentation.
Let me know if there are any further changes I need to make

!buildbot s390x Debian

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit a7bc0fe 🤖

The command will test the builders whose names match following regular expression: s390x Debian

The builders matched are:

• s390x Debian PR

Ah, I just realised that this was already in the awaiting merge stage but I requested a review from aisk... Sorry about that...