replace types of Union with Union of types in some places #22403
Conversation
|
Thanks for finding these. I would like to try to minimize use of |
OK, I didn't know there would be a difference for the compiler.
I started in a couple of places, but when the list of types is big, it start to be quite verbose... Please have a look at this patch in array.jl, and if it's fine by you, I will continue the transormation in the remaining places. |
base/random.jl
Outdated
| for i = 16*n128÷sizeof(T)+1:n | ||
| @inbounds A[i] = rand(r, T) | ||
| for T = (Base.BitInteger64_types..., Int128) | ||
| @eval function rand!(r::MersenneTwister, A::Array{$T}) |
base/array.jl
Outdated
| function ==(a::A, b::A) where A <: Union{Array{Int8,N},Array{UInt8,N},Array{Int16,N}, | ||
| Array{UInt16,N},Array{Int32,N},Array{UInt32,N}, | ||
| Array{Int64,N},Array{UInt64,N},Array{Int128,N}, | ||
| Array{UInt128,N}} where N |
There was a problem hiding this comment.
You can use something like const BitIntegerArray = Union{map(T->Array{T}, uniontypes(BitInteger))...}.
base/array.jl
Outdated
| const BitIntegerArray{N} = Union{map(T->Array{T,N}, BitInteger_types)...} where N | ||
| # use memcmp for == on bit integer types | ||
| function ==(a::Array{T,N}, b::Array{T,N}) where T<:BitInteger where N | ||
| function ==(a::A, b::A) where A <: BitIntegerArray |
There was a problem hiding this comment.
we don't use A for an array type in that many signatures, T or something a few characters longer would be a more common usage
There was a problem hiding this comment.
I found it useful for distinction as a composite type, but ok.
There was a problem hiding this comment.
So T was looking like aneltype, so I chose Arr; if still not good, please suggest.
There was a problem hiding this comment.
Arr works for me, it's not a variable name that we heavily use for array values
There was a problem hiding this comment.
(Quite an understatement as I could not find any use of Arr elsewhere in Base ;-) )
base/random.jl
Outdated
| rand!(rd::RandomDevice, A::BoolBitIntegerArray) = (win32_SystemFunction036!(A); A) | ||
|
|
||
| else # !windows | ||
|
|
There was a problem hiding this comment.
would be better to remove the blank line at the start of the if is_windows() block than add more at the beginning and end here - this is redundant with the indentation
There was a problem hiding this comment.
I usually prefer to have space before and after struct/method definitions for better readability (i.e. this if/else/end is not usual code), but your call.
There was a problem hiding this comment.
what isn't usual code about it?
There was a problem hiding this comment.
I just meant it's rare to have struct or method definition within an if block, it's usually at the top level; sometimes the style is amended for special cases, like skipping the indentation corresponding module blocks, I thought why not here, but no pb!
base/random.jl
Outdated
| end | ||
|
|
||
| function rand!(r::MersenneTwister, A::Array{T}) where T<:Union{Base.BitInteger64,Int128} | ||
| # A::Array{UInt128} will matche the specialized method above |
rfourquet
changed the title
|
Good to go? |
|
Will merge in a couple of days if no objection. |
T <: Union{A, B} is sometimes meant to mean either A or B,
which are concrete types. This can result in crashes
or wrong results if T takes actually the value Union{A, B}.
T <: Union{A, B}is sometimes meant to mean eitherAorB, which are concrete types. This can result in crashes or wrong results if T takes actually the valueUnion{A, B}.There may be other places in base with similar bugs...