-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
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. |
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.
I still think this was unnecessary, but I don't care enough to push back.
Next time though, please don't rush.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):