◐ Shell
reader mode source ↗
Skip to content

bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem()#11112

Merged
serhiy-storchaka merged 6 commits into
python:masterfrom
serhiy-storchaka:pydict-getitem
Feb 25, 2019
Merged

bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem()#11112
serhiy-storchaka merged 6 commits into
python:masterfrom
serhiy-storchaka:pydict-getitem

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Dec 11, 2018

Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

12 hidden conversations Load more…
@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ericsnowcurrently

Copy link
Copy Markdown
Member

Incidentally, how many uses of the non-WithError API remain after this?

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thank you for your review @ericsnowcurrently!

9 hidden conversations Load more…
@serhiy-storchaka

Copy link
Copy Markdown
Member Author

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.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

@brettcannon brettcannon changed the title bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem(). Feb 16, 2019
@serhiy-storchaka serhiy-storchaka merged commit a24107b into python:master Feb 25, 2019
@serhiy-storchaka serhiy-storchaka deleted the pydict-getitem branch February 25, 2019 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants