Skip to content
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

Jump to bottom

Remove dead code in _PyDict_GetItemHint #95948

Merged
merged 3 commits into from Aug 18, 2022
Merged

Remove dead code in _PyDict_GetItemHint #95948

merged 3 commits into from Aug 18, 2022

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Aug 13, 2022 •
edited

_PyDict_GetItemHint was an optimization that was subsequently made redundant.

So let's remove the dead code.

I am not removing _PyDict_GetItemHint completely here, because as far as I can tell it performs a different combination of error handling steps than the other ways to get items from a dict, and I don't feel confident rewriting the code in Python/specialize.c to work with the different assumptions around error handling.

So I am sticking with removing just the dead code, and a rename of the function (so that people get a proper error message when they are trying to still use the old version of _PyDict_GetItemHint).

If a reviewer has a hint for how to rewrite Python/specialize.c to use eg PyDict_GetItemWithError or PyDict_GetItem, I'd be more than happy to rewrite the PR. Thanks!

@matthiasgoergens
Remove dead code in _PyDict_GetItemHint
e51cc19 
_PyDict_GetItemHint was an optimization that was subsequently made
redundant.

So let's remove the dead code.

I am not removing _PyDict_GetItemHint completely here, because as far as
I can tell it performs a different combination of error handling steps
than the other ways to get items from a dict.
markshannon
markshannon reviewed Aug 15, 2022
View changes
Copy link
Member

@markshannon markshannon left a comment

Thanks for cleaning this up a bit.
I've a couple of suggestion for cleaning up the loose ends.

Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
markshannon
markshannon reviewed Aug 16, 2022
View changes
Python/specialize.c Outdated
_PyDict_GetItemHint(dict, name, -1, &value);
if (hint != (uint16_t)hint) {
Py_ssize_t index =
_PyDict_GetItemSpecialize(dict, name, &value);
Copy link
Member

@markshannon markshannon Aug 16, 2022

Choose a reason for hiding this comment

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

You forgot this one

Copy link
Contributor Author

@matthiasgoergens matthiasgoergens Aug 18, 2022 •
edited

Choose a reason for hiding this comment

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

Thanks for catching that! I was trying to be too quick.

I'm glad we have automated tests.

@markshannon
Copy link
Member

markshannon commented Aug 16, 2022

For future reference, if you force push then the reviewer has to re-review the whole PR.
If you push normally, then the reviewer just needs to check the additional changes.
This is a small PR, so it doesn't matter much here.

@methane
Copy link
Member

methane commented Aug 16, 2022 •
edited by markshannon

Have you checked that Cython doesn't use this API?

It has a leading underscore, so in theory, we are free to remove it.
Isn't documented, and there are no tests for it. So we don't even know if works.

@scoder?

@scoder
Copy link
Contributor

scoder commented Aug 18, 2022

The function is not used by Cython.

@matthiasgoergens
Copy link
Contributor Author

matthiasgoergens commented Aug 18, 2022

For future reference, if you force push then the reviewer has to re-review the whole PR. If you push normally, then the reviewer just needs to check the additional changes. This is a small PR, so it doesn't matter much here.

Noted. Sorry. I left the commit that you had already seen alone, and only force-pushed an amendment to the new commit. I had assumed github would be smart enough to do the Right Thing. I guess it's not.

Thanks for pointing that out!

@markshannon
Copy link
Member

markshannon commented Aug 18, 2022

Thanks @matthiasgoergens

@markshannon markshannon merged commit 4a6fa89 into python:main Aug 18, 2022
12 checks passed
@matthiasgoergens
Copy link
Contributor Author

matthiasgoergens commented Aug 18, 2022

Thanks @matthiasgoergens

Thank you for the quick review, and patience with me forgetting one rename!

I'm just getting started with contributing.

tiran pushed a commit to tiran/cpython that referenced this pull request Aug 19, 2022
@matthiasgoergens @tiran
Remove dead code in _PyDict_GetItemHint and rename to _PyDict_LookupI…
a9eba71 
…ndex (pythonGH-95948)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

@methane methane

Assignees
No one assigned
Labels
skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@matthiasgoergens @markshannon @methane @scoder @bedevere-bot @AlexWaygood