Skip to content

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jul 20, 2023

Mostly just a mechanical refactoring.

Copy link
Member

@gvanrossum gvanrossum left a 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);
Copy link
Member

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?

Copy link
Member Author

@brandtbucher brandtbucher Jul 20, 2023

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.

Comment on lines +95 to +98
_PyEval_FormatExcCheckArg(tstate, PyExc_UnboundLocalError,
UNBOUNDLOCAL_ERROR_MSG,
PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, oparg)
);
Copy link
Member

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).

Copy link
Member Author

@brandtbucher brandtbucher Jul 20, 2023

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.

Copy link
Member

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.

@brandtbucher brandtbucher enabled auto-merge (squash) July 20, 2023 20:07
@brandtbucher brandtbucher merged commit 8f4de57 into python:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants