Skip to content

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

Merged
adamreichold merged 3 commits intomainfrom
type-name-fast-path
Dec 19, 2023
Merged

Rename name to qualname and add name to better match Python and ensure abi3 fallbacks. #3660
adamreichold merged 3 commits intomainfrom
type-name-fast-path

Conversation

@adamreichold
Copy link
Member

@adamreichold adamreichold commented Dec 18, 2023

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

davidhewitt commented Dec 19, 2023

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

adamreichold commented Dec 19, 2023

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.

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