-
-
Notifications
You must be signed in to change notification settings - Fork 706
Refactor reference counting to use _Py_REFCNT and Py_SET_REFCNT #41137
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
Conversation
This change replaces direct access to ob_refcnt with proper API functions: - Use _Py_REFCNT() for reading reference counts - Use Py_SET_REFCNT() for setting reference counts This ensures compatibility with Python's free-threaded mode and follows Python's recommended practices for reference count manipulation. Changes: - src/sage/matrix/args.pxd: Use _Py_REFCNT instead of direct ob_refcnt access - src/sage/rings/integer.pyx: Use Py_SET_REFCNT for setting refcount - src/sage/rings/real_double.pyx: Use Py_SET_REFCNT for setting refcount - src/sage/symbolic/ginac/numeric.cpp: Refactor Py_INCREF/Py_DECREF macros Backported from passagemath/passagemath#1761
|
Documentation preview for this PR (built with commit cedb4c9; changes) is ready! 🎉 |
|
I am not too sure about _Py* - these are not for the public use, or at least they cannot be considered a stable API, no? @tobiasdiez ? |
|
tobiasdiez
left a comment
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 think it's public API, at least it's mentioned in the docs: https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#python-objects-as-parameters-and-return-values
It looks to me that the new Py_SET_REFCNT can be replaced by Py_INCREF/Py_DECREF, which are also available from cython.ref, and is a bit nicer.
But it does not like the original meaning. I think that The original meaning is to set the ref number is 1 |
It increases it from 0 to 1, or not? |
The annotation said So the ref count may be larger than 1 |
|
oh, misread this part. Sorry. But in ginac it is really just increasing & decreasing, right? |
It seems it redefine the Py_INCREF and Py_DECREF. I do not know why it needs to do that. I just preserve the original code's meaning. |
And before that, it undefine the two functions |
|
@dimpase help me review this. Thank you. It affect the compatibility of python 3.14 free-threading build |
|
@tobiasdiez Can you help me review this again. it undefine |
|
@tobiasdiez Thank you for reviewing |
sagemathgh-41137: Refactor reference counting to use _Py_REFCNT and Py_SET_REFCNT This change replaces direct access to ob_refcnt with proper API functions: - Use _Py_REFCNT() for reading reference counts - Use Py_SET_REFCNT() for setting reference counts This ensures compatibility with Python's free-threaded mode and follows Python's recommended practices for reference count manipulation. <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#41137 Reported by: Chenxin Zhong Reviewer(s): Tobias Diez
|
This now broke all the uv-based workflows: https://github.com/sagemath/sage/actions/runs/19874021091/job/56956791524#step:8:2159 could you please have a look @cxzhong |
OK. I notice that. Look that cython/cython@b684235 |
Can you help me upgrade the uv lock file? |
This change replaces direct access to ob_refcnt with proper API functions:
This ensures compatibility with Python's free-threaded mode and follows Python's recommended practices for reference count manipulation.
📝 Checklist
⌛ Dependencies