Skip to content

Conversation

@davidhewitt
Copy link
Member

While considering CI failures in #5481 I realised our way of defining static "slots" is potentially questionable.

  • We use a & immutable reference in the static, which seems questionable given the intent is to express mutable data. We cast this away using pointer casting.
  • The reference seems to also requires us to read the mutable static's value to cast to a pointer, hence the unsafe block. Reading the value of mutable statics is known to be full of footguns.

The cleanest way I could find to do this, as proposed in this PR, is to store the mutable static as a static mut [T; N] array. It requires specifying N, because the compiler does not allow inference of the array size.

The nice thing about storing slots as an array is that we can then get a pointer to its first element with addr_of_mut!(SLOTS).cast(), which avoids any unsafe block or reading the value of the static.

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.

I was having similar thoughts when I saw that failure yesterday. I think using an array here is much more appropriate and less sketchy. Having to type out the length is a bit annoying, but still quite readable I would say.

@davidhewitt davidhewitt added this pull request to the merge queue Oct 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 5, 2025
@davidhewitt
Copy link
Member Author

Hmm annoyingly this looks like another thing blocked on #5486

@Icxolu
Copy link
Contributor

Icxolu commented Oct 5, 2025

That's unfortunate... I guess it's the reason we had the reference in there to begin with.

@Icxolu Icxolu enabled auto-merge October 21, 2025 20:19
@Icxolu Icxolu added this pull request to the merge queue Oct 21, 2025
Merged via the queue into PyO3:main with commit 67da00d Oct 21, 2025
43 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