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.

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

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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