bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem()#11112
bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem()#11112serhiy-storchaka merged 6 commits into
Conversation
ericsnowcurrently
left a comment
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.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: |
Sorry, something went wrong.
|
Incidentally, how many uses of the non- |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you for your review @ericsnowcurrently!
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I have made the requested changes; please review again. |
Sorry, something went wrong.
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
Sorry, something went wrong.
https://bugs.python.org/issue35459