GeezerVison is showing github.com — gh-128715: Expose ctypes.CField, with info attributes by encukou · Pull Request #128950 · python/cpython
Conversation
encukou
commented
Jan 17, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Expose the type of struct/union field descriptors (_ctypes._CField) as ctypes.CField. (This is mainly to make it easier to refer to it -- for typing and documentation. Creating CFields manually is not supported.)
Add attributes for easier introspection:
name & type, as defined in _fields_
byte_size & byte_offset, specify the chunk of memory to read/write
bit_size & bit_offset, which specify the shift & mask for bitfields
is_bitfield & is_anonymous booleans
The existing offset remains, as an alias for byte_offset.
The existing size is unchanged. Usually it is the same as byte_size, but for bitfields, it contains a bit-packed combination of bit_size & bit_offset.
Update the repr() of CField.
Use the same values internally as well. (Except bit_size, which might overflow Py_ssize_t for very large types. Instead, in C use bitfield_size, which is 0 for non-bitfields and thus serves as is_bitfield flag. Different name used clarity.)
Old names are removed from the C implementation to ensure a clean transition.
For simplicity, I keep byte_size in CFieldObject for now, even though it's redundant: it's the size of the underlying type.
Add a generic test that ensures the values are consistent, and that we don't have overlapping fields in structs. Use this check throughout test_ctypes, wherever a potentially interesting Structure or Union is created. (Tests for how simple structs behave after they're created don't need the checks, but I erred on the side of adding checks.)
Lift the restriction on maximum field size that was temporarily added in GH-126938. The max size is now Py_ssize_t.
This PR does not yet touch cfield.c getters & setters: the bit-packed “size” argument is computed and passed to those.
📚 Documentation preview 📚: https://cpython-previews--128950.org.readthedocs.build/
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
picnixz reacted with heart emoji
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Add alignment requirement
- Mention that ob_size is unreliable if you don't control it
- Add some links for context
- basicsize should include the base type in generaly not just PyObject
This adds a “by-the-way” link to `PyObject_New`, which shouldn't be
used for GC types. In order to be comfortable linking to it, I also
add a link to `PyObject_GC_New` from its docs. And the same for
`*Var` variants, while I'm here.
|
Should I review what you wrote now or do you prefer me to wait?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Sorry, I missed the comment! I'd appreciate a review.
The failure on 32-bit Debian is due to an existing bug, but such an exotic one that it's not worth fixing now: #130410
(It shows that the tests work, though!)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@picnixz, should I wait for your review?
No pressure, feel free to say no :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Oh I forgot... I'll have maybe a bit of time before taking my plane or I'll be back on Wednesday. If you want to wait until then, I'll review it otherwise just go ahead!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Some comments (hard to do a better review on a phone)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you for the review!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks for the reviews everyone!
I'll merge tomorrow if there are no objections.
|
sergey-miryanov reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 think I've much more to say. My main concern was the unicode-exactness of the name which is now resolved so I'm fine.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Almost tomorrow.... :)
Thank you for the reviews, everyone!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
diegorusso
pushed a commit
to diegorusso/cpython
that referenced
this pull request
Apr 1, 2025
…-128950)
- Restore max field size to sys.maxsize, as in Python 3.13 & below
- PyCField: Split out bit/byte sizes/offsets.
- Expose CField's size/offset data to Python code
- Add generic checks for all the test structs/unions, using the newly exposed attrs
Reviewers
picnixz
picnixz approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|