Skip to content

remove Deref implementations for old "gil refs" types #4593

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.

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

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Oct 4, 2024

Follow-up on #4589 (comment)

I'm open to whether this needs a CHANGELOG entry... I guess it does because it can affect type bounds (e.g. as it does in this PR). Maybe that's a good reason to park this until 0.24 as a low-ish priority (breaking) change, that's easier the longer ago GIL Refs died.

Comment on lines +277 to 279
#[repr(transparent)]
#[allow(clippy::upper_case_acronyms)]
pub struct $name($crate::PyAny);
Copy link
Member Author

Choose a reason for hiding this comment

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

Without adding the #[repr] the types started raising dead code warnings for the contents never being read.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess an alternative would maybe be

#[non_exhaustive]
pub struct $name;

But I'm not sure if there are subtle semver differences between these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another alternative is to get rid of the create_exception! macro if we can come up with a complete error handling plan, which was started in #4186 and #4413 I guess...

@Icxolu
Copy link
Contributor

Icxolu commented Oct 4, 2024

Yeah, it probably deserves a changelog entry. Targeting 0.24 seems fine to me. Do we want to add a milestone to track that?

@davidhewitt davidhewitt added this to the 0.24 milestone Oct 4, 2024
@davidhewitt
Copy link
Member Author

Good idea, milestone created, will leave this in draft until then I guess.

@davidhewitt davidhewitt marked this pull request as draft October 4, 2024 11:51
@davidhewitt davidhewitt marked this pull request as ready for review March 7, 2025 08:20
@davidhewitt
Copy link
Member Author

Ok, CHANGELOG added, maybe we merge this just in time for 0.24? :)

@davidhewitt davidhewitt mentioned this pull request Mar 7, 2025
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@Icxolu Icxolu enabled auto-merge March 7, 2025 18:14
@Icxolu Icxolu added this pull request to the merge queue Mar 7, 2025
Merged via the queue into PyO3:main with commit bdc372f Mar 7, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants