-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
GH-106701: Move _PyUopExecute
to Python/executor.c
#106924
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
Conversation
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.
No need for a re-review if you decide to take my suggestion.
void _PyEval_FormatKwargsError(PyThreadState *tstate, PyObject *func, PyObject *kwargs); | ||
PyObject *_PyEval_MatchClass(PyThreadState *tstate, PyObject *subject, PyObject *type, Py_ssize_t nargs, PyObject *kwargs); | ||
PyObject *_PyEval_MatchKeys(PyThreadState *tstate, PyObject *map, PyObject *keys); | ||
int _PyEval_UnpackIterable(PyThreadState *tstate, PyObject *v, int argcnt, int argcntafter, PyObject **sp); |
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.
Didn't you say previously that these would have to be PyAPI_FUNC
?
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.
Not yet, no. That can be added once we actually need the symbols to be exported.
_PyEval_FormatExcCheckArg(tstate, PyExc_UnboundLocalError, | ||
UNBOUNDLOCAL_ERROR_MSG, | ||
PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, oparg) | ||
); |
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 think it would be fine to call _PyEval_FormatExcUnbound
here and at the corresponding point in ceval.c, then you won't have to move UNBOUNDLOCAL_ERROR_MSG
into the header file (which feels a little unprincipled).
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 can do that, sure.
FWIW though, NAME_ERROR_MSG
is already in ceval_macros.h
, so it's not super out of place there. In fact, it might be nice to put NAME_ERROR_MSG
, UNBOUNDFREE_ERROR_MSG
, and UNBOUNDLOCAL_ERROR_MSG
together in the same place.
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, all together in the same place is good, so then I vote for moving all to the header and not changing the code at the label.
Mostly just a mechanical refactoring.
_PyUopExecute
) to a new file #106701