Skip to content

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

Merged
miss-islington merged 10 commits into
python: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 into
python:mainfrom
tiran:gh-96046-check_ht_cached_keys

Conversation

@tiran

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

Copy link
Copy Markdown
Member

@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

markshannon commented Aug 18, 2022

Copy link
Copy Markdown
Member

Want to make this a real PR?

@tiran

tiran commented Aug 18, 2022

Copy link
Copy Markdown
Member Author

Want to make this a real PR?

Sure! Do you agree with the proposed solution?

@markshannon

markshannon commented Aug 18, 2022

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

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

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

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

tiran commented Aug 18, 2022

Copy link
Copy Markdown
Member Author

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

@markshannon

markshannon commented Aug 19, 2022

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

@vstinner vstinner 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.

LGTM.

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

@vstinner

vstinner commented Aug 19, 2022

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

markshannon commented Aug 19, 2022

Copy link
Copy Markdown
Member

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

vstinner
vstinner approved these changes Aug 19, 2022
View reviewed changes

@vstinner vstinner 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.

LGTM.

Comment thread Objects/typeobject.c
@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

miss-islington commented Aug 22, 2022

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

bedevere-bot commented Aug 22, 2022

Copy link
Copy Markdown

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