-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-129594: Remove redundant check on varargs in _PyArg_CheckPositional #129595
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.
This check is indeed unnecessary but can you transform it into an assertion? just so that we're sure we don't call the function incorrectly or is it impossible to add some assertion inside this?
I think we could use an assertion within the macro. But if there is no reason to have it, it can only cause confusion. The code in question was moved around a bit by Victor Stinner (only refactoring, functionality was not touched afaics). |
I'll merge this one tomorrow, sorry for forgetting about this one :') |
The static PyObject *
sys_audit(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
{
PyObject *return_value = NULL;
const char *event;
PyObject *__clinic_args = NULL;
if (!_PyArg_CheckPositional("audit", nargs, 1, PY_SSIZE_T_MAX)) {
goto exit;
} In the situation above, we want to check that nargs is at least 1. However, because of the So I don't think we need the check at all as it doesn't help us (and even slows us down when |
arf, I forgot to merge this one. @eendebakpt can you confirm my analysis? I may have done it while I was just waking up and now I'm about to sleep :') (sorry, tomorrow it will be merged!) |
Don't ask for analysis and promise to merge tomorrow, you might end up in an awkward situation ;-) I agree with your analysis. The cpython/Tools/clinic/libclinic/parse_args.py Line 100 in cf8941c
cpython/Tools/clinic/libclinic/parse_args.py Line 495 in cf8941c
|
Ok thanks. To summarize |
|
This was caused by a general GitHub outage: https://www.githubstatus.com/incidents/d0nm3xcdc5jw |
varargs
in_PyArg_CheckPositional
#129594