gh-117139: Add header for tagged pointers#118330
gh-117139: Add header for tagged pointers#118330Fidget-Spinner merged 19 commits intopython:mainfrom
Conversation
colesbury
left a comment
There was a problem hiding this comment.
The names of some of the functions could use some work:
Py_STACKREF_TAG
Py_STACKREF_TAG_DEFERRED
Py_STACKREF_UNTAG_BORROWED
Py_STACKREF_UNTAG_OWNED
Maybe the following:
// instead of Py_STACKREF_TAG
// Converts a PyObject * to a PyStackRef, stealing the reference
PyStackRef_StealRef(PyObject*) -> PyStackRef
// instead of Py_NewRef_StackRef(Py_STACKREF_TAG_DEFERRED(...));
// Converts a PyObject * to a PyStackRef, with a new reference
PyStackRef_NewRef(PyObject*) -> PyStackRef
// instead of Py_STACKREF_UNTAG_BORROWED
PyStackRef_Get(PyStackRef) -> PyObject *
// or maybe
PyStackRef_GET(PyStackRef) -> PyObject *
// instead of Py_STACKREF_UNTAG_OWNED
PyStackRef_StealObject -> PyObject *
I don't think we should have a function that only does what Py_STACKREF_TAG_DEFERRED currently does -- instead oI think we should have a function that effectively does the combo Py_NewRef_StackRef(Py_STACKREF_TAG_DEFERRED()) but more efficiently.
Additionally, instead of Py_STACKREF_TAG(NULL) let's have a macro for the constant, i.e. something like #define Py_STACKREF_NULL ({_PyStackRef) { .bits = 0 });
Include/internal/pycore_tagged.h
Outdated
| static inline int | ||
| _PyObject_HasDeferredRefcount(PyObject *op) | ||
| { | ||
| #ifdef Py_GIL_DISABLED | ||
| return (op->ob_gc_bits & _PyGC_BITS_DEFERRED) != 0; | ||
| #else | ||
| return 0; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Minor, but can we keep the _PyObject_HasDeferredRefcount and _PyObject_SetDeferredRefcount functions in pycore_object.h? We can #include pycore_object.h in pycore_tagged.h if needed.
- Keeps accessors for
ob_gc_bitsin the same file _PyStackRefuse is mostly limited to the interpreter and some supporting functions
There was a problem hiding this comment.
I tried this in the original branch but we can't easily for some reason. We can't #include pycore_object.h in pycore_tagged.h because it causes anything that includes pycore_interp.h to fail to compile. Not sure why.
There was a problem hiding this comment.
Ah, my old enemy...circular includes. When this has happened to me in the past, I solved this by splitting out the problematic parts of the header file into a new "pycore_..._state.h". It holds just the structs needed by pycore_interp.h. Then pycore_interp.h includes only the "_state.h" file. You can see examples in Include/internal.
|
I adopted your naming scheme for all but the following:
|
|
Note to self: when merging this, to add Sam as co-author. |
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
colesbury
left a comment
There was a problem hiding this comment.
- Would you please clean-up the formatting of the functions? I gave one example of function below.
- Let's revert the move of
_PyObject_SetDeferredRefcount. From my local testing, it doesn't look like it's necessary in this PR. If we run into circular imports, we can do a better refactoring as they come up with @ericsnowcurrently's suggestion.
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
colesbury
left a comment
There was a problem hiding this comment.
A few more minor comments, but otherwise LGTM.
I'm still not thrilled with the names we have (I know, I suggested a bunch of them...), but given that it's an internal-only header we can update them later if we come up with something better.
Right now we have four conversion functions:
PyObject *PyStackRef_Get(_PyStackRef)PyObject *PyStackRef_StealObject(_PyStackRef)_PyStackRef PyStackRef_StealRef(PyObject *)_PyStackRef PyStackRef_NewRefDeferred(PyObject *)
There's a lot of similarity between them, but I don't think the naming conventions currently do a good job of sigifying that.
|
|
||
| #define PyStackRef_XSETREF(dst, src) \ | ||
| do { \ | ||
| _PyStackRef *_tmp_dst_ptr = &(dst) \ |
There was a problem hiding this comment.
There is a missing semicolon here but I don't want to waste CI resources, so I will fix this in another PR, since nothing is using this file at the moment anyways.
--------- Co-authored-by: Sam Gross <655866+colesbury@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.