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

nightly CI failures due to finalizers on BigFloats #1825

Closed
lgoettgens opened this issue Oct 2, 2024 · 5 comments · Fixed by #1829
Closed

nightly CI failures due to finalizers on BigFloats #1825

lgoettgens opened this issue Oct 2, 2024 · 5 comments · Fixed by #1829

Comments

@lgoettgens
Copy link
Collaborator

Just as a heads-up and point to reference to. The issue is already reported upstream at JuliaLang/julia#55965

@thofma
Copy link
Member

thofma commented Oct 2, 2024

Not sure if being mutable is part of the API. Anyway, we can adjust the test.

@lgoettgens
Copy link
Collaborator Author

According to the comment in the test file, this test should test JuliaLang/julia#26939. But since this was about BigFloats, and BigFloats can seemingly no longer be used as keys in WeakKeyIdDicts, I think we should just remove the test

@lgoettgens
Copy link
Collaborator Author

Hmm, so JuliaLang/julia#55906 changed the corresponding test in julia base for WeakKeyDict to test with BigInts rather than BigFloats (which then no longer corresponds to JuliaLang/julia#26939...). Let's wait for an answer in the julia issue

@fingolfin
Copy link
Member

Well we had that test in there precisely because someone was trying to use a BigFloat as a key there... the old BigFloat implementation had a bug there, which was fixed.

Perhaps WeakKeyDict and WeakKeyIdDict should be enhanced to also support immutable keys (to prevent breakage when changing the implementation of a type, like happened here).

But this all really isn't our concern: we don't use bigfloat keys in weak key (id) dicts, and frankly I don't see why anyone would. So I'd just remove the failing test and be done with this.

@fingolfin
Copy link
Member

(Actually I am not even sure it would be possible to have WeakKeyDict work with an immutable key, but again, that's really not important for us here)

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 a pull request may close this issue.

3 participants