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.

Sign up for GitHub

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

gh-96046: Initialize ht_cached_keys in PyType_Ready() #96047

Merged
merged 10 commits into from
Aug 22, 2022
Merged

gh-96046: Initialize ht_cached_keys in PyType_Ready() #96047

merged 10 commits into from
Aug 22, 2022

Conversation

tiran
Copy link
Member

@tiran tiran commented Aug 17, 2022 •
edited by miss-islington
Loading

@tiran

This comment was marked as outdated.

Sign in to view
@tiran tiran changed the title gh-96046: Check managed dict types have ht_cached_keys gh-96046: Initialize ht_cached_keys in PyType_Ready() Aug 17, 2022
@markshannon
Copy link
Member

Want to make this a real PR?

@tiran
Copy link
Member Author

tiran commented Aug 18, 2022

Want to make this a real PR?

Sure! Do you agree with the proposed solution?

@markshannon
Copy link
Member

Yes. PyType_Ready() is the best place to do this initialization.

There are additional sanity checks I want to do, but I'll do those in a separate PR.

@tiran tiran marked this pull request as ready for review August 18, 2022 11:50
@tiran tiran requested a review from markshannon as a code owner August 18, 2022 11:51
@tiran tiran added the needs backport to 3.11 only security fixes label Aug 18, 2022
@tiran tiran requested a review from pablogsal August 18, 2022 11:52
Fidget-Spinner
Fidget-Spinner approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM but we need @vstinner to test if this works with his reproducer.

Objects/typeobject.c Show resolved Hide resolved
@tiran tiran mentioned this pull request Aug 18, 2022
Closed
vstinner
vstinner reviewed Aug 18, 2022
View reviewed changes
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Core and Builtins/2022-08-18-13-47-59.gh-issue-96046.5Hqbka.rst Show resolved Hide resolved
@tiran
Copy link
Member Author

tiran commented Aug 18, 2022

@vstinner Could you please create change proposals for your requests. That why I can easily commit them through GH UI.

@markshannon
Copy link
Member

This seems to have suffered scope creep.

Would it be possible to split it into two PRs, one with the original changes and one with all the changes to check_basicsize_includes_size_and_offsets?
The reason I ask is that the original changes need backporting to 3.11 and the additional changes do not.

vstinner
vstinner previously approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

IMO the NEWS entry belongs to the C API category, rather than Core and Builtins.

@vstinner
Copy link
Member

Hum, why did you revert changes, like using PyErr_NoMemory()?

@vstinner vstinner dismissed their stale review August 19, 2022 10:51

the PR changed since I reviewed it

@markshannon
Copy link
Member

@tiran Could you put the MemoryError back?
Otherwise, LGTM.

vstinner
vstinner approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

Objects/typeobject.c Show resolved Hide resolved
@vstinner vstinner mentioned this pull request Aug 20, 2022
Closed
@miss-islington miss-islington merged commit 53e6a9a into python:main Aug 22, 2022
@miss-islington
Copy link
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@tiran tiran deleted the gh-96046-check_ht_cached_keys branch August 22, 2022 05:24
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 22, 2022
@bedevere-bot
Copy link

GH-96164 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 22, 2022
@tiran @miss-islington
pythongh-96046: Initialize ht_cached_keys in PyType_Ready() (pythonGH…
9776890 
…-96047)

(cherry picked from commit 53e6a9a)

Co-authored-by: Christian Heimes <christian@python.org>
miss-islington added a commit that referenced this pull request Aug 22, 2022
@miss-islington @tiran
gh-96046: Initialize ht_cached_keys in PyType_Ready() (GH-96047)
3ea0beb 
(cherry picked from commit 53e6a9a)

Co-authored-by: Christian Heimes <christian@python.org>
rwgk pushed a commit to rwgk/pybind11_scons that referenced this pull request Aug 22, 2022
As used for retesting:
9168ad0 
pybind/pybind11#4142 (comment)

main: python/cpython#96047
3.11: python/cpython#96164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@vstinner vstinner vstinner approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@pablogsal pablogsal Awaiting requested review from pablogsal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tiran @markshannon @vstinner @miss-islington @bedevere-bot @Fidget-Spinner