bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() #11112
Conversation
There was a problem hiding this comment.
First of all, thanks for working on this! :) Overall it looks good.
My main concern with this PR is changing semantics. From what I can tell you're introducing a bunch of changes in behavior, albeit it corner error cases. What is the risk to compatibility? My gut tells me there's at least a slight risk.
Secondly, you've touched a lot of critical code. Please make sure to run the benchmark suite to ensure the PR doesn't slow down Python. :)
Also, there a number of places where I wanted to suggest a better spelling. However, such changes would be slightly riskier and would mostly clutter up the PR, obscuring the core changes. So I've left out those comments and focused mostly on checking correctness.
Finally, the most likely thing I might have missed in this review is refcounts. You've added quite a few places that exit early when there's an error. I'm not sure that I checked to make sure everything was properly decref'ed in those new error cases.
Modules/_sre.c
Outdated
| PyExc_IndexError, | ||
| "no such group" | ||
| ); | ||
| if (index < 0) { |
There was a problem hiding this comment.
Was this change intentional? At first glance it looks like the code you've removed actually matters.
Is this function only ever called with a pre-validated index (e.g. the one returned from match_getindex())? If so, it would be helpful to have a comment here indicating that validation of the index must be done by the caller. And if that's the case then why have this check (and short-circuit) here?
There was a problem hiding this comment.
The removed code was moved into match_getindex() because it was repeated after every call of match_getindex(). This function is only called with valid index or the result of match_getindex(). This check was here just to make the caller place simpler. Will move it to the caller place and add an assert instead.
| @@ -1692,7 +1700,7 @@ MODULE_INITFUNC(void) | |||
| } | |||
| } | |||
| Py_DECREF(errmod_name); | |||
| model_module = PyDict_GetItem(d, modelmod_name); | |||
| model_module = PyDict_GetItemWithError(d, modelmod_name); | |||
There was a problem hiding this comment.
Doesn't the error need to be returned or cleared?
There was a problem hiding this comment.
It is returned below, at line 1713.
There was a problem hiding this comment.
The model_module gets reset on line 1705, so the error from the first attempt may remain uncleared or swallowed
There was a problem hiding this comment.
Line 1705 is executed only no error was raised at this line.
Error handling in this function (as well as in many other module initialization functions) is pretty poor. Results of PyModule_AddObject() and derived functions are not checked, and references are leaked in case of error. But this is different issue(s).
There was a problem hiding this comment.
Then shouldn't line 1704 have && !PyErr_Occurred() like line 1694 does?
Objects/dictobject.c
Outdated
| int status = PyDict_SetItem(d, key, value); | ||
| if (status < 0) { | ||
| if (override || PyDict_GetItemWithError(d, key) == NULL) { | ||
| if ((!override && PyErr_Occurred()) || PyDict_SetItem(d, key, value) < 0) { |
There was a problem hiding this comment.
This is a little hard to read. There's a lot going on in these two lines. Perhaps split it up a little?
| @@ -4797,14 +4824,12 @@ static int | |||
| add_methods(PyTypeObject *type, PyMethodDef *meth) | |||
| { | |||
| PyObject *dict = type->tp_dict; | |||
| PyObject *name; | |||
There was a problem hiding this comment.
I'd put this down where it's first used (i.e. right above line 4852 if (isdescr) {).
|
When you're done making the requested changes, leave the comment: |
|
Incidentally, how many uses of the non- |
Modules/_sre.c
Outdated
| PyExc_IndexError, | ||
| "no such group" | ||
| ); | ||
| if (index < 0) { |
There was a problem hiding this comment.
The removed code was moved into match_getindex() because it was repeated after every call of match_getindex(). This function is only called with valid index or the result of match_getindex(). This check was here just to make the caller place simpler. Will move it to the caller place and add an assert instead.
| @@ -1692,7 +1700,7 @@ MODULE_INITFUNC(void) | |||
| } | |||
| } | |||
| Py_DECREF(errmod_name); | |||
| model_module = PyDict_GetItem(d, modelmod_name); | |||
| model_module = PyDict_GetItemWithError(d, modelmod_name); | |||
There was a problem hiding this comment.
It is returned below, at line 1713.
|
As for benchmarks, running the benchmark suite exposes some slowdown on some tests, but results can have significant random component. I'll research this in more details to get more trustworthy result. |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
brettcannon
changed the title
https://bugs.python.org/issue35459