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-127443: Add semantics for stolen references in Doc/data/refcounts.dat #127468

Closed
wants to merge 7 commits into from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Dec 1, 2024

For now, it only changes the entry that are know to be stealing references. I'd like to have some help here: if you know which functions steals references, just drop the name and I'll update the PR (it's hard for me to find them automatically...).

This PR should be merged after #127451 and probably before #127443 so that I can use the new symbol and don't need to update the comment.


📚 Documentation preview 📚: https://cpython-previews--127468.org.readthedocs.build/

@picnixz
Copy link
Member Author

picnixz commented Dec 1, 2024

Some functions that steal references but are not yet included/fixed (waiting for other PRs):

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

I will say, there are some functions that do truly steal a reference by DECREFing the input. For example, PyModule_AddObject--do we want to treat that with $, or with -1?

@picnixz
Copy link
Member Author

picnixz commented Dec 2, 2024

If they DECREF the input maybe a -1 + comment is better? or maybe $$ that it stole even more things!

@ZeroIntensity
Copy link
Member

I'll let you decide :)

@encukou
Copy link
Member

encukou commented Dec 4, 2024

I will say, there are some functions that do truly steal a reference by DECREFing the input. For example, PyModule_AddObject--do we want to treat that with $, or with -1?

On success, PyModule_AddObject steals a reference in the usual way: it does PyModule_AddObjectRef (+1) and Py_DECREF (-1).

However, on error it does not steal a reference -- it does failed PyModule_AddObjectRef (+0) and skips the decref. Still ±0 as far as refcounts.dat is concerned, but the caller ends up owning the reference in this case.
I assume most of the +1 for arguments are incorrect in error cases.
Adding stolen is an improvement; IMO we should also add borrowed where we know an argument is not stolen/weird, so it's possible to know what's been checked.

@encukou
Copy link
Member

encukou commented Dec 4, 2024

Would it make sense to prefix existing comments with #, and leave field 5 to annotations like stolen?
Existing tools (if there are any) would treat the whole thing as a comment.

@picnixz
Copy link
Member Author

picnixz commented Jan 2, 2025

After a discussion with Petr, I think it would better not to introduce new semantics for now. The idea is to make the last field hold some special attributes. Those attributes would indicate stolen. This is also better to do it like this because existing tools (if any) may fail to parse $ tokens (technically... our own Sphinx extension failed on them).

So I'll close this one but will definitely re-use the data from here.


Note: I could limit myself to modifying the 5th field, but until I've written the tool and the correct syntax I expect, I prefer minimizing the commits.

@picnixz picnixz closed this Jan 2, 2025
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