GeezerVison

It would be nice to have this backported to 3.8 and 3.7, so that fixes using it may be backported as well. I'm not sure if that's appropriate though, so maybe somebody else can make the call.

@vstinner Do you mind taking a peek at this when you have a free minute?

Pre-emptively backporting wouldn't be OK, but it could be considered if there was a bug fix backport that depended on it.

Do you still need this, though? It looks like most of the cleanup PRs have already been merged.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ncoghlan I've edited the PR to include a few of the many modules that still need to be refactored. The linked PRs on the issue were only the ones that would not benefit from this addition. I estimate that there are several dozen more. Hopefully it's clear that this function would make the bugfixes much more straightforward, enough to backport the function itself for their use.

@vstinner I too prefer it when functions don't steal references. If I were designing a brand new API, I would definitely not do it this way. The issue is that there are dozens of modules with complicated (broken) initialization code that relies on the stealing behavior of PyModule_AddObject (but doesn't properly handle errors). See _ctypes for a good example of code that is much easier to fix if we keep the stealing behvior.

If you still disagree, I can change it to never steal. It's just my impression from looking at the mountain of complex, broken code ahead of me that this is probably the easier route to take, especially since this API is just meant to be a "band-aid" for another broken API.

I didn't expect the Spanish Inquisition!

Nobody expects the Spanish Inquisition!

@vstinner: please review the changes made to this pull request.

Looks like this PR can be closed after victor merged this PR: #23122 @brandtbucher

@brandtbucher:

@vstinner I too prefer it when functions don't steal references. If I were designing a brand new API, I would definitely not do it this way. The issue is that there are dozens of modules with complicated (broken) initialization code that relies on the stealing behavior of PyModule_AddObject (but doesn't properly handle errors). See _ctypes for a good example of code that is much easier to fix if we keep the stealing behvior.

If you still disagree, I can change it to never steal. It's just my impression from looking at the mountain of complex, broken code ahead of me that this is probably the easier route to take, especially since this API is just meant to be a "band-aid" for another broken API.

I added PyModule_AddObjectRef() which uses strong references, rather than only stealing a reference on success.

I also enhanced the documentation to show concrete examples:
https://docs.python.org/dev/c-api/module.html#c.PyModule_AddObjectRef

I modified a few extension to use PyModule_AddObjectRef(). Sometimes, PyModule_AddObject() is more appropriate. Sometimes, PyModule_AddObjectRef() is more appropriate. Both functions are relevant, and I don't see a clear winner.

I agree than fixing existing code is painful, but I hope that new code using mostly PyModule_AddObjectRef() would be simpler to review. I'm not sure that it's simpler to write new code using PyModule_AddObjectRef(), since you might need more Py_DECREF() calls.

My intent is to have more "regular" code about reference counting. See also: https://bugs.python.org/issue42294

Since you wrote that this API is a band aid on a broken API, I consider that you are fine with rejecting it and move on to the new PyModule_AddObjectRef().

Anyway, thanks for you attempt to make the C API less broken :-)