Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-116437: Use new C API PyDict_Pop() to simplify the code#116438
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
It also fixes leaks in |
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.
Maybe replace== -1 with< 0 to check for failure.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM.
I don't know if< 0 to check for errors is a common idiom, but I prefer to use it rather than== 0. Currently,https://devguide.python.org/developer-workflow/c-api/index.html#guidelines-for-expanding-changing-the-public-api only mentions "return -1: internal error or API misuse; exception raised".
Thank you for your review Victor. |
bedevere-bot commentedMar 7, 2024
|
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.
Sorry for the post-merge review. I was curious about what changed and happened to notice a few things.
| if (PyDict_DelItemString(dct, "pwritev") == -1) { | ||
| PyErr_Clear(); | ||
| if (PyDict_PopString(dct, "pwritev", NULL) < 0) { | ||
| return -1; | ||
| } | ||
| if (PyDict_DelItemString(dct, "preadv") == -1) { | ||
| PyErr_Clear(); | ||
| if (PyDict_PopString(dct, "preadv", NULL) < 0) { | ||
| return -1; |
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 is slightly different behavior, isn't it? Before, we tried removing each item and ignoredall errors. Now we fail at the first error. Is that okay? (Maybe@ronaldoussoren has some thoughts on this?) Was this intentional?
(The same applies to the changes in Modules/timemodule.c.)
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 ignoring errors is always a good thing.
| int rc = PyDict_Pop(remaining_dict, name, &value); | ||
| Py_DECREF(name); | ||
| if (rc < 0) { | ||
| goto cleanup; | ||
| } | ||
| if (!value) { | ||
| if (PyErr_Occurred()) { | ||
| goto cleanup; | ||
| } | ||
| break; | ||
| } | ||
| if (PyList_Append(positional_args, value) < 0) { | ||
| rc = PyList_Append(positional_args, value); | ||
| Py_DECREF(value); | ||
| if (rc < 0) { | ||
| goto cleanup; | ||
| } | ||
| if (PyDict_DelItem(remaining_dict, name) < 0) { | ||
| goto cleanup; | ||
| } | ||
| Py_DECREF(name); |
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.
Good catch about fixing the decrefs.
| if (PyDict_DelItemString(dict, "__file__")) { | ||
| PyErr_Clear(); | ||
| if (PyDict_PopString(dict, "__file__", NULL) < 0) { | ||
| PyErr_Print(); | ||
| } | ||
| if (PyDict_DelItemString(dict, "__cached__")) { | ||
| PyErr_Clear(); | ||
| if (PyDict_PopString(dict, "__cached__", NULL) < 0) { | ||
| PyErr_Print(); | ||
| } |
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.
Nice. UsingPyErr_Print() here makes sense.
| swap_current_task(asyncio_state *state, PyObject *loop, PyObject *task) | ||
| { | ||
| PyObject *prev_task; | ||
| if (task == Py_None) { | ||
| if (PyDict_Pop(state->current_tasks, loop, &prev_task) < 0) { | ||
| return NULL; | ||
| } | ||
| if (prev_task == NULL) { | ||
| Py_RETURN_NONE; | ||
| } | ||
| return prev_task; | ||
| } |
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.
It took me a minute to make sense of your changes in this file, but now I get it. 👍
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.
Same here. I blocked here for 5 minutes :-D
Uh oh!
There was an error while loading.Please reload this page.