bpo-26515: Update C API docs to use PyModuleDef_Init() by berkerpeksag · Pull Request #8682 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a nice improvement to me. There's a forward reference that I don't think is quite right any more, and one small syntactic tweak needed to fix the build.
| } |
|---|
| then we need to specify the :c:func:`spam_exec` function for the |
| c:var:`Py_mod_exec` slot:: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the leading : here (that appears to be what the CI error is complaining about)
| @@ -234,8 +244,8 @@ needed to ensure that it will not be discarded, causing :c:data:`SpamError` to |
|---|
| become a dangling pointer. Should it become a dangling pointer, C code which |
| raises the exception could cause a core dump or other unintended side effects. |
| We discuss the use of ``PyMODINIT_FUNC`` as a function return type later in this |
| sample. |
| We discuss the use of :c:macro:`PyMODINIT_FUNC` as a function return type later |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the modified example, this macro hasn't actually been referenced yet. Instead, the forward reference needs to explain that this list of slots will then be added to a PyModuleDef struct defining the module as a whole, which is described later.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for getting to this so late -- I missed the review request notification.
| @@ -204,24 +204,34 @@ usually declare a static object variable at the beginning of your file:: |
|---|
| static PyObject *SpamError; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem if the module is loaded more than once (for example, in multiple subinterpreters).
In that case, spam_exec would be executed multiple times, each time calling PyErr_NewException and setting SpamError to the newly created error object.
This pointer needs to be part of per-module state.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
And if you don't make the requested changes, you will be put in the comfy chair!
@berkerpeksag It looks like this one was close to being merged. When you have a chance, please review the changes requested by @ncoghlan and @encukou. Thanks!
I'll update my branch this weekend. Sorry for my late response!
More detailed discussion/insructions is in PEP 630.
@encukou so is this still needed or is PEP 630 the main reference point now?
Because of PEP 630's open issues, making PyModuleDef_Init the default way to make classes would be leading some users into a trap. I am still not comfortable making it a default in the docs, and I'd rather have users read PEP 630 and understand the benefits and tradeoffs.
Until the open issues are solved, of course.
Thank you. Closing this for now then.