Repository navigation
BLD: only use PyObject_HEAD if PyObject isn't opaque - #20492
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
PyObject_HEAD if PyObject isn't opaque
1019969 to
9b41c09
Compare
9b41c09 to
29512d8
Compare
|
This doesn't feel like it would actually work if you tried to run it to me. Just as an example: astropy/astropy/utils/xml/src/iterparse.c Line 960 in 51a6654 The cast here is dodgy. You've allocated a Python object so that object starts with the PyObject head. So the cast to an object without the head ignores that structure. The correct line would be: (Assuming no inheritance... With inheritance And you should be very suspicious of functions taking astropy/astropy/utils/xml/src/iterparse.c Line 1122 in 51a6654 The size needs to be negative in the opaque case. |
|
If the comments above aren't sufficiently obvious then I can make a PR with one file in a working state. The other files should be pretty straightforward once you've need the pattern. Or I can elaborate. |
|
Thanks @da-woods for reviewing this. Indeed I did not attempt to validate the validity of my patch at runtime, as I only focused on getting past a build-time error, but this approach seems naive in retrospect. If you have time for it, I would indeed appreciate seeing a PR from you to fix a single file; I'd feel much more confident that we can get the rest of them right without assistance then :) |
Description
close #19479
ref implementation: cython/cython#7412
I got started working on abi3t-compliance, and can confirm this patch alone resolves most existing build-time errors, but it isn't sufficient in itself so I'm not going to add this target to (extra) CI yet.
AI Disclosure
N/A
Merge method