Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-97588: Move ctypes struct/union layout logic to Python #123352

Merged
merged 95 commits into from
Sep 5, 2024

Conversation

encukou
Copy link
Member

@encukou encukou commented Aug 26, 2024

As discussed in #97702, moving this logic to Python should make it easier to fix the various struct layout bugs in ctypes.

This PR should be strictly a refactoring of the code in main, except the _ctypes.CField class can now be instantiated from Python.

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of comments on the C side. I'll do the Python files now.

Modules/_ctypes/cfield.c Show resolved Hide resolved
Modules/_ctypes/cfield.c Outdated Show resolved Hide resolved
Modules/_ctypes/cfield.c Outdated Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Outdated Show resolved Hide resolved
Modules/_ctypes/stgdict.c Outdated Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Outdated Show resolved Hide resolved
@picnixz picnixz self-requested a review August 27, 2024 15:50
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you construct the layout depends on whether it's a union or a struct and whether it's gcc or ms layout. I'm not sure whether it's easier to maintain one huge function or if you want to create separate factories. AFACIT, it could be easier for instance to have a function that determines whether it's a gcc layout or not, one function that is responsible for parsing _align_ and one function responsible for the _pack_ attribute. For the rest, they can be kept inside the get_layout function.

For the loop over the input fields, I don't know whether you want to keep a single one or factor it out (it could be clearer to review, but it will likely cause more code so I'm fine with the current implementation).

Lib/test/test_ctypes/test_bitfields.py Outdated Show resolved Hide resolved
Lib/test/test_ctypes/test_bitfields.py Show resolved Hide resolved
Lib/ctypes/_layout.py Outdated Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Outdated Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Outdated Show resolved Hide resolved
Lib/ctypes/_layout.py Outdated Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Copy link
Member Author

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review!

Modules/_ctypes/cfield.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Modules/_ctypes/stgdict.c Outdated Show resolved Hide resolved
Modules/_ctypes/stgdict.c Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
@picnixz picnixz self-requested a review August 28, 2024 15:32
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Show resolved Hide resolved
Lib/ctypes/_layout.py Outdated Show resolved Hide resolved
Modules/_ctypes/cfield.c Show resolved Hide resolved
Modules/_ctypes/cfield.c Show resolved Hide resolved
Lib/test/test_ctypes/test_bitfields.py Show resolved Hide resolved
@encukou
Copy link
Member Author

encukou commented Sep 2, 2024

Thank you for the reviews!
I plan to merge after 3.13 rc2 is out, and then start solving the layout bugs and missing tests :)

@encukou
Copy link
Member Author

encukou commented Sep 4, 2024

RC2 is delayed. I'll merge now, if buildbots pass.

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 4, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 96a5c0d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 4, 2024
@picnixz
Copy link
Contributor

picnixz commented Sep 4, 2024

RC2 is delayed. I'll merge now, if buildbots pass.

The "now" being in at least 8 hours 😆

@encukou encukou merged commit ce9f84a into python:main Sep 5, 2024
104 of 107 checks passed
@encukou encukou deleted the ctypes-pypack branch September 5, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants