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

Rename name to qualname and add name to better match Python and ensure abi3 fallbacks. #3660

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Rename name to qualname and add name to better match Python and ensure abi3 fallbacks. #3660

merged 3 commits into from
Dec 19, 2023

Conversation

adamreichold
Copy link
Member

No description provided.

@adamreichold adamreichold added the CI-skip-changelog Skip checking changelog entry label Dec 18, 2023
@adamreichold adamreichold force-pushed the type-name-fast-path branch 2 times, most recently from 283c850 to 1e26957 Compare December 18, 2023 09:34
@adamreichold adamreichold changed the title While having a abi3 fallback is good, we can keep the fast path for non-abi3 builds. Add PyType::full_name which is tp_name and has an abi3 fallback. Dec 18, 2023
@adamreichold adamreichold removed the CI-skip-changelog Skip checking changelog entry label Dec 18, 2023
@adamreichold adamreichold mentioned this pull request Dec 18, 2023
Merged
@adamreichold adamreichold force-pushed the type-name-fast-path branch 2 times, most recently from 532a3a9 to 68e2049 Compare December 18, 2023 09:43
adamreichold
adamreichold commented Dec 18, 2023
View reviewed changes
src/types/typeobject.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

Possibly relevant here: we should check out PyType_GetName and PyType_GetQualName added in 3.11. Maybe they solve the abi3 case (for 3.11 and up)?

@adamreichold
Copy link
Member Author

Possibly relevant here: we should check out PyType_GetName and PyType_GetQualName added in 3.11. Maybe they solve the abi3 case (for 3.11 and up)?

For name, it would work by using GetQualName. But for full_name we are still missing the __module__ part which is also derived from tp_name. I am not sure I can get that from elsewhere? Will certainly push a commit to use those two instead of attribute look-ups where applicable.

davidhewitt
davidhewitt reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Yep, I think all of this makes sense, thanks 👍

Just a few short comments, and one slight concern about lifetime of the borrow.

src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
@adamreichold adamreichold changed the title Add PyType::full_name which is tp_name and has an abi3 fallback. Rename name to qualname and add name to better match Python and ensure abi3 fallbacks. Dec 19, 2023
@adamreichold adamreichold force-pushed the type-name-fast-path branch 2 times, most recently from d521542 to 61b1c0d Compare December 19, 2023 14:29
davidhewitt
davidhewitt approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the revisions here; this worked out more fiddly than I think either of us expected 👀

@adamreichold adamreichold force-pushed the type-name-fast-path branch 2 times, most recently from aad5d51 to a1490ff Compare December 19, 2023 15:36
@adamreichold adamreichold added this pull request to the merge queue Dec 19, 2023
Merged via the queue into main with commit 87e42c9 Dec 19, 2023
35 checks passed
@adamreichold adamreichold deleted the type-name-fast-path branch December 19, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@adamreichold @davidhewitt