gh-91719: Add pycore_opcode.h internal header file #91906
Conversation
Move the following API from Include/opcode.h (public C API) to a new Include/internal/pycore_opcode.h header file (internal C API): * EXTRA_CASES * _PyOpcode_Caches * _PyOpcode_Deopt * _PyOpcode_Jump * _PyOpcode_OpName * _PyOpcode_RelativeJump
|
@markshannon @gvanrossum @encukou: IMO in Python 3.11, Include/opcode.h leaks way too many implementation details. "_Py" private symbols must be moved to the internal API. This PR moves private names to the internal C API. In Python 3.10, Include/opcode.h already contains _PyOpcode_RelativeJump and _PyOpcode_Jump which are short tables. But "EXTRA_CASES" macro name is not prefixed by "Py", "PY", "_Py" or "_PY" and so can override an existing name if opcode.h is included in a third party C extension. Hopefully, |
I wouldn't say must, but it definitely makes things clearer. |
|
@vstinner Why did you do this in such a hurry? I was behind on code review and you merged this without any review. None of the remaining macros in this file start (the opcode names, HAS_CONST etc.) start with Maybe this would have been a better candidate for a semi-public API. |
I merged a similar change (commit 20cc695 adds another internal header file) to fix an issue on FreeBSD, I merged this PR to avoid merge conflicts.
These macros exist in Python 2.7 and seem safe to be used outside Python. Moreover, the Python opcode module provide a similar API in Python. _PyOpcode_Caches, _PyOpcode_Deopt, _PyOpcode_RelativeJump, _PyOpcode_Jump, EXTRA_CASES are private, not documented, and seem to be tighly coupled to ceval.c. Do you see any reason to keep them in the public C API?
In that case, they should drop their "_" prefix (ex: rename _PyOpcode_Deopt to PyOpcode_Deopt), be documented, and maybe even have tests. But the semi-stable API doesn't exist yet, it's this draft PR: #91789 I agree that maybe something should be added to the semi-stable API, that's why I put @encukou in the loop ;-) Is there any usage outside Python code base for these variables? I don't think that EXTRA_CASES should be used outside ceval.c. |
Oh sorry, next time I will wait longer. |
|
More generally, last years, it's rare that I get any review on my pull requests even if I wait for 1 week to 1 month. I got used to review my changes myself. But for some PRs, I like to ping a few persons who worked on code around my changes to notify them, in the secret hope that maybe, one day, they will have a look, even if it's a "post-commit review" :-) I usually wait a few hours after creating the PR. Sometimes, I "sleep on my PR": wait at least one night to get a fresh point of view. Later, when I review my change, usually it helps to discover issues. Also, when I review a PR on GitHub, I spot more issues, than when I read a change on my terminal. Maybe because I'm less distracted and I focus on the change ;-) |
|
Your time scale is way too aggressive. I understand you're impatient but I can't always find time to review a PR right away. I try not to get backed up more than a week, but waiting a few hours and then assuming that nobody is interested is unacceptable, sorry. |
|
Python 3.11 beta1 is scheduled for Friday, 2022-05-06: next week. I would prefer to not have to change the C API after beta1. I didn't notice these new private functions earlier. There are now guidelines for the C API to define what should be put in which API: https://devguide.python.org/c-api/ Is there any reason to use one of the macro/variable that I moved to the internal C API outside CPython (ceval.c, code/frame objects) itself? Is there something wrong with my PR? Last years, I almost never got any review on my changes to change/reorganize the C API. Here I pinged you to notify you about the change, I didn't expect a review. But I like post-commit reviews. Well, I like reviews in general :-) |
|
I am uncomfortable with post-commit reviews. A few core devs still use this style: merge, then wait for review. I don't like it. All I am saying is that I need time to think through the consequences and I haven't found that time yet -- instead we're arguing about process. IMO there was nothing wrong with the status quo that required committing in a hurry. |
| @@ -15,6 +15,7 @@ | |||
| #include "pycore_long.h" // _PyLong_GetZero() | |||
| #include "pycore_object.h" // _PyObject_GC_TRACK() | |||
| #include "pycore_moduleobject.h" // PyModuleObject | |||
| #include "pycore_opcode.h" // EXTRA_CASES | |||
There was a problem hiding this comment.
I don't know why you call out EXTRA_CASES in the comment -- this file uses several other tables defined there as well.
There was a problem hiding this comment.
When I add an #include, I put the same of a single symbol get from this #include, even if many other symbols are used. I only do that to check time to time if the include is still used: here I just have check if EXTRA_CASES is still used in the file. Otherwise, for each #include, I have to remove the line, try to build, and check if the C file still builds successfully.
There was a problem hiding this comment.
Would be nice if there was tooling that did this instead. Without the comment.
Move the following API from Include/opcode.h (public C API) to a new
Include/internal/pycore_opcode.h header file (internal C API):