Skip to content

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

Merged
miss-islington merged 10 commits intopython:mainfrom
tiran:gh-96046-check_ht_cached_keys
Aug 22, 2022
Merged

gh-96046: Initialize ht_cached_keys in PyType_Ready()#96047
miss-islington merged 10 commits intopython:mainfrom
tiran:gh-96046-check_ht_cached_keys

Conversation

@tiran
Copy link
Copy Markdown
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
Copy Markdown
Member

markshannon commented Aug 18, 2022

Want to make this a real PR?

@tiran
Copy link
Copy Markdown
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
Copy Markdown
Member

markshannon commented Aug 18, 2022

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
Copy Markdown
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.

@tiran tiran mentioned this pull request Aug 18, 2022
Closed
vstinner
vstinner reviewed Aug 18, 2022
View reviewed changes
@tiran
Copy link
Copy Markdown
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
Copy Markdown
Member

markshannon commented Aug 19, 2022

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
Copy Markdown
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
Copy Markdown
Member

vstinner commented Aug 19, 2022

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
Copy Markdown
Member

markshannon commented Aug 19, 2022

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

vstinner
vstinner approved these changes Aug 19, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@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
Copy Markdown
Contributor

miss-islington commented Aug 22, 2022

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
Copy Markdown

bedevere-bot commented Aug 22, 2022

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 added a commit to rwgk/pybind11_scons that referenced this pull request Aug 22, 2022
@rwgk
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

@vstinner vstinner vstinner approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner 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