GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-117139: Add header for tagged pointers by Fidget-Spinner · Pull Request #118330 · python/cpython Conversation
|
| 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.
The reason will be displayed to describe this comment to others. Learn more.
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.
ob_gc_bits in the same file_PyStackRef use is mostly limited to the interpreter and some supporting functions
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I adopted your naming scheme for all but the following:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Note to self: when merging this, to add Sam as co-author. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-Authored-By: Sam Gross <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
_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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-Authored-By: Sam Gross <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
| #define PyStackRef_XSETREF(dst, src) \ | ||
| do { \ | ||
| _PyStackRef *_tmp_dst_ptr = &(dst) \ |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
--------- Co-authored-by: Sam Gross <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.