Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-134819: Add sys.set_object_tags and sys.get_object_tags#135073
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
corona10 commented Jun 3, 2025
@vstinner@colesbury@Fidget-Spinner@ZeroIntensity |
Python/sysmodule.c Outdated
| { | ||
| assert(object!=NULL); | ||
| if (strcmp(tag, "immortal") ==0){ | ||
| _Py_SetImmortal(object); |
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.
This definitely isn't safe on its own. (Trust me, I've gone down quite the rabbithole in getting arbitrary object immortalization working. It's extraordinarily complex to do safely.)
corona10Jun 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm, do you think that we should not allow setting "immortal" tag at this moment?
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.
Yeah, at least for now. sys.set_object_tags is allowed to ignore tags, right?
corona10Jun 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, we will ignore (and it's intended behavior)
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jun 3, 2025
As I wrote in the issue, I dislike this API and I prefer the current design: one function per object attribute, such as |
corona10 commented Jun 3, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
The problem is that the alternative implementation also needs to provide the same functions to pass the unittests and avoid people from checking attributes based on the CPython-only function. So this API will remove unnecessary additional APIs that are only for checking the CPython implementation detail. I also dislike that we are exposing CPython implementation detail to the pure Python function. |
corona10 commented Jun 3, 2025
So if you think that we're fine with exposing CPython implementation details through Python and C API. I can happily drop this proposal. |
colesbury commented Jun 3, 2025
I agree with @vstinner here and still feel the same way as in my comment on the issue. I am also not in a rush to expose these APIs at the Python level. |
ZeroIntensity commented Jun 3, 2025
I'm indifferent. I see arguments for both sides that both make sense. In general, I think we do need a way to expose unstable APIs to Python the same way we do in C. It's definitely not fun to have to set up a C extension just so you can get something that is useful at a Python level (e.g., deferred reference counting). I think Donghee makes a good point that prefixing with |
Uh oh!
There was an error while loading. Please reload this page.