WIP: Allow convert and reinterpret for Arrays of unions with missing/nothing #36186
Conversation
oxinabox
changed the title
oxinabox
force-pushed
the
ox/unionconv
branch
2 times, most recently
from
1dcb9b1
to
da23ed5
Compare
remove mistakenly included files remove mistakenly included files
| ==# | ||
| end | ||
| # Resolve ambiguity | ||
| #Base.convert(::Type{Array{Any,1}}, xs::Array{Any,1}) = xs |
There was a problem hiding this comment.
I don't know why this is ambiguous, and the answer would probably go a long way to explaining why this breaks so much
| @@ -69,6 +69,33 @@ convert(::Type{T}, x::T) where {T>:Union{Missing, Nothing}} = x | |||
| convert(::Type{T}, x) where {T>:Missing} = convert(nonmissingtype_checked(T), x) | |||
| convert(::Type{T}, x) where {T>:Union{Missing, Nothing}} = convert(nonmissingtype_checked(nonnothingtype_checked(T)), x) | |||
|
|
|||
| for Cs in ((:Missing,), (:Nothing,), (:Missing, :Nothing)) | |||
| @eval function Base.reinterpret( | |||
There was a problem hiding this comment.
| @eval function Base.reinterpret( | |
| @eval function reinterpret( |
|
So turns out there a a ton of problems with doing reinterprete this way (at least for now) like leaking memory. So I am just going to leave this to sit here and people can take my test cases for when it is done the right way |
|
In general, we want to move away from reinterpret—it has many issues. What you really want is a conversion that's very cheap (i.e. free), which has the desired semantics. |
Ok, I thought it made sense to have a |
| xs::Array{Union{T, $(Cs...)},N} | ||
| ) where {T,N} | ||
| isbitstype(T) || throw(ArgumentError("cannot reinterpret `$(eltype(xs))`, type `$(T)` is not a bits type")) | ||
| return unsafe_wrap(Array{T,N}, Ptr{T}(pointer(xs)), length(xs)) |
There was a problem hiding this comment.
What happens when xs get's garbage collected? Doesn't this mess with TBAA?
There was a problem hiding this comment.
idk, I didn't even know we did aliasing detection.
|
I think the only way this could be reasonably done is to "move" (think |
Then it can't be called |
|
It would be something like |
Closes #26681 and #31152
Right now convert kind of breaks the compiler and i am not sure why.
Debugging that is pending, so right now it is commented out so i could run the
reinterpettestsWriting this is made harder by #36185