bpo-35381 Remove all static state from posixmodule #15892
Conversation
|
I pushed a few changes directly rather than writing a comment about them. Hope that's OK with you :) |
I'm proposing a PyState_AddModule doc update to make things clearer: python#16101
|
I added some commits here. The core dev sprint is ending, and I won't have time to look at this issue for the next few days (or weeks, unfortunately, depending on work load). |
|
@encukou thanks for all the reviews! I'll update this to address all the comments |
Agreed, thanks for the fix!
I think there's two options: 1) Solve this by creating a new C-API that handles this without a A more systematic solution would be to implement another C-API to hide the abstraction there. That being said, I rather not expand the C-API given that this pattern is barely present in the stdlib. I've kept the change for now to
Unfortunately, under PEP384,
Thanks for merging them! |
As we remove statics, there will need to be a generic solution for _Py_IDENTIFIER. Eric & the team are thinking about how to handle them.
PEP 384, the stable ABI, is not relevant to the standard library. |
|
Hey Petr, good to hear back from you!
Outside of the special case of
It is very relevant to any C Extension Module! That's something that I've been trying to get across but I'm probably not doing a good job at it 😛 The really nice benefit of PEP384 is that it hides all the implementation details of CPython. This allows any other implementation to use these modules as well. So adding these changes here allows 1) to set a pattern for other C Extensions to use PEP384, and 2) enables these extensions to be used by any other implementation. Let me know if it makes sense and/or if you have other questions/concerns!
Agreed! Any suggestions on the proper avenues to do that? |
|
Sorry for the delay – I needed to cancel my CPython day last week :(
Agreed. However, the set a pattern for other C Extensions is an issue here. Can we merge this PR with commit c1a6d03 reverted, and leave that bit for future work?
Brainstorming ideas:
|
Sounds like a plan! I'll update this PR in a bit :) |
|
@encukou done! |
| @@ -537,7 +538,7 @@ _Py_Uid_Converter(PyObject *obj, void *p) | |||
| if (index == NULL) { | |||
| PyErr_Format(PyExc_TypeError, | |||
| "uid should be integer, not %.200s", | |||
| Py_TYPE(obj)->tp_name); | |||
| _PyType_Name(Py_TYPE(obj))); | |||
There was a problem hiding this comment.
tp_name and _PyType_Name() are not the same. There were some discussions between fully qualified names (module.name1.name2.name3) vs short name (name3), but no consensus was found. Well, I should reopen a discussion ;-) I prefer to provide longest names in error messages to ease debug.
There was a problem hiding this comment.
@vstinner I actually debated over that for a bit! I think we are missing something like: PyType_QualName. I can do a quick PR for that but I don't know if there will be push back from other devs. Thoughts?
There was a problem hiding this comment.
Reopening the discussion sounds like a good idea.
There was a problem hiding this comment.
Was it reopened? A discussion could be useful in https://bugs.python.org/issue42035.
After python#9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: python#10854. All the issues in that commit have now been addressed since python#11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou
After python#9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque. The original PR that got messed up a rebase: python#10854. All the issues in that commit have now been addressed since python#11661 got committed. This change also removes any state from the data segment and onto the module state itself. https://bugs.python.org/issue35381 Automerge-Triggered-By: @encukou
After #9665, this moves the remaining types in posixmodule to be heap-allocated to make it compatible with PEP384 as well as modifying all the type accessors to fully make the type opaque.
The original PR that got messed up a rebase: #10854. All the issues in that commit have now been addressed since #11661 got committed.
This change also removes any state from the data segment and onto the module state itself.
https://bugs.python.org/issue35381
Automerge-Triggered-By: @encukou