RFC: redefine PyTypeObjects as heap types for Limited API compliance by neutrinoceros · Pull Request #154 · brandon-rhodes/python-sgp4

neutrinoceros · 2026-03-11T09:48:22Z

based off #153
needed for #151

At the time of opening, I'm getting a single compilation error:

clang++: warning: optimization flag '-ffloat-store' is not supported [-Wignored-optimization-argument]
extension/wrapper.cpp:582:10: error: use of undeclared identifier 'Py_tp_as_sequence'
  582 |         {Py_tp_as_sequence, &SatrecArray_as_sequence},

I don't have experience with "sub slots", which seem needed for defining the tp_as_sequence slot correctly. Hopefully I can crack it soon but in the mean time, any help would be appreciated.

neutrinoceros · 2026-03-11T10:01:43Z

maybe the rabbit hole goes a lot deeper. I think this is just the first symptom of a much broader issue: SatrecArrayType is the first type of variable lenght that I attempt to convert to a heap type. I'm almost sure it should be possible, but I may be slow to reach a conclusion/solution.

neutrinoceros · 2026-03-11T10:34:39Z

FWIW I doubt that this change can be handled simply without dropping Python 2.7 too, but I also don't have a simple way to check.

neutrinoceros · 2026-03-11T16:38:10Z

... which might not be a problem since, IIUC, the extension is only ever compiled for Python 3 anyway

neutrinoceros · 2026-03-11T16:49:44Z

I got it to compile locally. Hopefully it runs smoothly in CI too.

neutrinoceros · 2026-04-19T21:48:11Z

failure is real. I'll probably need to iterate a bit if I cannot reproduce this locally (which remains to be seen)

neutrinoceros · 2026-04-20T07:47:37Z

I can reproduce, this shouldn't take long

neutrinoceros · 2026-04-20T08:01:14Z

shotgun debugging: it seems that this is the block that errors out within the Limited API build

        if (!PyObject_IsInstance(item, (PyObject*) &SatrecType)) {
            PyErr_Format(PyExc_ValueError, "every item must be a Satrec,"
                         " but element %d is: %R", i, item);
            Py_DECREF(item);
            return -1;
        }

from
SatrecArray_init(SatrecArrayObject *self, PyObject *args, PyObject *kwds)

brandon-rhodes

Thanks again for all of these PRs, which I'll try to make better progress through in May than I did in April. Here are a few questions about this one. Once they're answered, I think it will be ready to merge! This was interesting to read, as it's my first introduction to the new mechanisms for defining types.

brandon-rhodes · 2026-04-29T09:53:48Z

-    Py_INCREF(&SatrecType);
-    if (PyModule_AddObject(m, "Satrec", (PyObject *) &SatrecType) < 0) {
+    if (PyModule_AddObject(m, "Satrec", SatrecType) < 0) {
        Py_DECREF(&SatrecType);


I think you also need a Py_DECREF(&SatrecArrayType); here, since otherwise you will leak that object by losing the only pointer to it?

I think that's correct. I also conclude that I can collapse two now-identical blocks.

brandon-rhodes · 2026-04-29T09:55:28Z

-        extra_compile_args=['-ffloat-store'],
+        extra_compile_args=[
+            '-ffloat-store',
+            "-std=c++20" if get_default_compiler() != "msvc" else "/std:c++20",


Let's add a comment explaining why this is necessary, and why both compilers wouldn't choose the most recent C++ automatically.

Sure. I'll probably need to revert this part just refresh the logs, because I don't recall the specifics now.

see this log: https://github.com/brandon-rhodes/python-sgp4/actions/runs/25109895590/job/73581108136?pr=154

extension/wrapper.cpp(22): warning C4200: nonstandard extension used: zero-sized array in struct/union extension/wrapper.cpp(22): note: This member will be ignored by a defaulted constructor or copy/move assignment operator extension/wrapper.cpp(202): warning C4244: 'initializing': conversion from 'long' to 'char', possible loss of data extension/wrapper.cpp(565): error C7555: use of designated initializers requires at least '/std:c++20' extension/wrapper.cpp(583): error C7555: use of designated initializers requires at least '/std:c++20' error: command 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.44.35207\\bin\\HostX86\\x86\\cl.exe' failed with exit code 2

I don't know why msvc doesn't pick c++20 on its own though.

(I added this line back on my latest push so that you can merge this if you want)

If -std=c++20 doesn't seem to be needed, then it would be nice to adjust this so that it's not supplied; I'd rather not introduce arguments that aren't necessary.

If you could, adjust the if to be positive rather than negative; that's a Raymond Hettinger piece of advice (if I recall?) that does tend to make things more readable for me in the future.

Thanks for digging out those logs! I can add a comment after merging to explain to myself why we have to adjust the MS compile argument.

Ah, a fellow Hettinbro !
hat tip

neutrinoceros · 2026-04-29T10:10:25Z

Answering from my phone right now. 3/4 of your questions require my double-checking. I'll try to squeeze this into my afternoon work session !

brandon-rhodes · 2026-04-29T17:18:47Z

-        extra_compile_args=['-ffloat-store'],
+        extra_compile_args=[
+            '-ffloat-store',
+            "-std=c++20" if get_default_compiler() != "msvc" else "/std:c++20",


If -std=c++20 doesn't seem to be needed, then it would be nice to adjust this so that it's not supplied; I'd rather not introduce arguments that aren't necessary.

If you could, adjust the if to be positive rather than negative; that's a Raymond Hettinger piece of advice (if I recall?) that does tend to make things more readable for me in the future.

Thanks for digging out those logs! I can add a comment after merging to explain to myself why we have to adjust the MS compile argument.

brandon-rhodes · 2026-04-29T17:21:40Z

-    Py_INCREF(&SatrecArrayType);
-    if (PyModule_AddObject(m, "SatrecArray", (PyObject *) &SatrecArrayType) < 0) {
+    if ((PyModule_AddObject(m, "Satrec", SatrecType) < 0)
+        || (PyModule_AddObject(m, "SatrecArray", SatrecArrayType) < 0)) {


Alas, I think this introduces an error in the case that the first PyModule_AddObject() succeeds but the second fails: in that case, we wind up calling Py_DECREF(&SatrecType); with the result that SatrecType is freed, even though it's still a member of the module with the name "Satrec". When the module is the finalized and discarded, I think it would experience a double-free?

Ah, that makes sense. Sorry I didn't think of that.

brandon-rhodes · 2026-04-29T17:26:11Z

+    if ((PyModule_AddObject(m, "Satrec", SatrecType) < 0)
+        || (PyModule_AddObject(m, "SatrecArray", SatrecArrayType) < 0)) {
        Py_DECREF(&SatrecArrayType);
        Py_DECREF(&SatrecType);


As I re-read all these decref's, I'm confused about why they say things like Py_DECREF(&SatrecType); rather than Py_DECREF(SatrecType);. Isn't SatrecType already a pointer? With the & added, it looks like it would be trying to free the pointer rather than the object?

you're right. I don't know where this mistake came from. I probably was thinking compilation would just fail if I passed the wrong type. I'm actually not sure why it didn't. How is PyObject** acceptable where a PyObject* is expected ? (curious to know if you got an explanation for this, but I'll fix this in any case)

I was asking myself exactly the same question as I was reading it! 🙂 I don't know why the compiler didn't flag it.

brandon-rhodes · 2026-04-29T17:26:53Z

Thanks so much for the forward progress on this! I've made three final comments which I think should resolve the remaining snags that I might see in the code.

neutrinoceros · 2026-04-29T20:05:03Z

Great, thank you so much for you careful review. I think all problems you spotted should be addressed now !

neutrinoceros force-pushed the rfc/heaptypes branch from 7124b3e to dc0a5ac Compare March 11, 2026 16:49

neutrinoceros force-pushed the rfc/heaptypes branch from dc0a5ac to d96ef3f Compare March 11, 2026 16:51

This was referenced Mar 11, 2026

BLD/WHL: declare Python 3 wheels as abi3-compliant #151

Merged

WHL: systematically run tests on wheel artifacts #158

Closed

neutrinoceros force-pushed the rfc/heaptypes branch from d96ef3f to 2210fd6 Compare April 19, 2026 21:08

neutrinoceros marked this pull request as ready for review April 19, 2026 21:08

neutrinoceros marked this pull request as draft April 19, 2026 21:14

neutrinoceros force-pushed the rfc/heaptypes branch from 2210fd6 to 570c2e8 Compare April 20, 2026 08:14

neutrinoceros commented Apr 20, 2026

View reviewed changes

Comment thread extension/wrapper.cpp

neutrinoceros force-pushed the rfc/heaptypes branch from 406d027 to 02e0e6d Compare April 20, 2026 08:27

neutrinoceros marked this pull request as ready for review April 20, 2026 08:27

neutrinoceros marked this pull request as draft April 20, 2026 08:37

neutrinoceros force-pushed the rfc/heaptypes branch from 37685d9 to 8d1c87b Compare April 20, 2026 08:48

neutrinoceros marked this pull request as ready for review April 20, 2026 08:48

brandon-rhodes requested changes Apr 29, 2026

View reviewed changes

neutrinoceros force-pushed the rfc/heaptypes branch from a355508 to f919e72 Compare April 29, 2026 12:50

neutrinoceros marked this pull request as draft April 29, 2026 12:51

neutrinoceros force-pushed the rfc/heaptypes branch from 87a55b1 to 212ce31 Compare April 29, 2026 12:59

neutrinoceros marked this pull request as ready for review April 29, 2026 13:48

brandon-rhodes requested changes Apr 29, 2026

View reviewed changes

RFC: redefine PyTypeObjects as heap types for Limited API compliance

18395c0

neutrinoceros force-pushed the rfc/heaptypes branch from 212ce31 to 18395c0 Compare April 29, 2026 20:03

brandon-rhodes merged commit 4db47a0 into brandon-rhodes:master Apr 29, 2026
21 checks passed

neutrinoceros deleted the rfc/heaptypes branch April 29, 2026 20:38

Uh oh!

Conversation

neutrinoceros commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Mar 11, 2026

Uh oh!

neutrinoceros commented Mar 11, 2026

Uh oh!

neutrinoceros commented Mar 11, 2026

Uh oh!

neutrinoceros commented Mar 11, 2026

Uh oh!

neutrinoceros commented Apr 19, 2026

Uh oh!

neutrinoceros commented Apr 20, 2026

Uh oh!

neutrinoceros commented Apr 20, 2026

Uh oh!

Uh oh!

brandon-rhodes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandon-rhodes commented Apr 29, 2026

Uh oh!

neutrinoceros commented Apr 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

neutrinoceros commented Mar 11, 2026 •

edited

Loading