-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Allow reinterpreting singleton types #43500
Conversation
Hmm. It's hard to say whether we should allow this. The divide-by-zero error should be avoided at least. But then if we want to allow this, it should be implemented inside |
To implement it inside Line 1217 in 98b485e
Additionally, I remembered that singleton types were primarily used for function types, and I don't know if giving a meaning to |
From triage: |
Thank you for the triage and the review! I added a commit with the fix: doing Let me know if there is anything else I can do. |
CI failure seems unrelated. |
d49d605
to
987cc1a
Compare
base/reinterpretarray.jl
Outdated
@@ -475,6 +497,10 @@ end | |||
v = convert(T, v)::T | |||
# Make sure to match the scalar reinterpret if that is applicable | |||
if sizeof(T) == sizeof(S) && (fieldcount(T) + fieldcount(S)) == 0 | |||
if Base.issingletontype(T) # singleton types | |||
@boundscheck checkbounds(a, i1, tailinds...) | |||
return T.instance # setindex! is a noop here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return T.instance # setindex! is a noop here | |
return a # setindex! is a noop here |
This function is supposed to return the argument, though looks like several of the cases are wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry about that! I fixed both instances in e63a8fb.
I also fixed the currently erroneous return value for setindex!
on some reinterpretarray
s where it would return the parent array:
julia> t = reinterpret(Int, UInt[15])
1-element reinterpret(Int64, ::Vector{UInt64}):
15
julia> setindex!(t, 7, 1) # should return t instead of t.parent
1-element Vector{UInt64}:
0x0000000000000007
That's a bugfix (I believe) but it's technically breaking so let me know if you prefer I'd move that to another PR.
@Liozou Can you rebase on the latest master? That should fix the |
e63a8fb
to
cda45f4
Compare
Done, there is a remaining CI failure for the Sockets test but it looks unrelated. |
This PR (or one of the two follow ups, not 100% sure) allows julia> a = Int8[]
Int8[]
julia> ms = reinterpret(Missing, a)
0-element reinterpret(Missing, ::Vector{Int8})
julia> push!(a, 1)
1-element Vector{Int8}:
1
julia> ms
1-element reinterpret(Missing, ::Vector{Int8}):
missing This was encountered during a discussion on discourse, about reinterpreting empty views/arrays. As 1.8 has not been released, perhaps this can be decided soon-ish? Possible solution: disallow |
Fix #43403. It does not actually allow reinterpreting all 0-sized types, only the immutable ones (aka singleton types), but I think that's what was intended anyway (see also #17149).
I'm not sure whether it should be merged, considering what @vtjnash said in #34865 (comment), but I'm opening the PR in case it is decided it is worth adding.
Since
reinterpret
is a fairly basic building block, there is also the potential issue of slowing down critical paths. I don't think there is any harm done in this regard since thesizeof(T) == 0
andisdefined(T, :instance)
conditions should all be eliminated at compilation I think, but I'm no expert on this.