Skip to content

Commit

Permalink
fix bug with max_values in union! (#30315)
Browse files Browse the repository at this point in the history
  • Loading branch information
rfourquet authored and JeffBezanson committed Dec 11, 2018
1 parent caa0273 commit f49cb42
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
6 changes: 4 additions & 2 deletions base/abstractset.jl
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,11 @@ function union!(s::AbstractSet, sets...)
end

max_values(::Type) = typemax(Int)
max_values(T::Type{<:Union{Nothing,BitIntegerSmall}}) = 1 << (8*sizeof(T))
max_values(T::Union) = max(max_values(T.a), max_values(T.b))
max_values(T::Union{map(X -> Type{X}, BitIntegerSmall_types)...}) = 1 << (8*sizeof(T))
# saturated addition to prevent overflow with typemax(Int)
max_values(T::Union) = max(max_values(T.a), max_values(T.b), max_values(T.a) + max_values(T.b))
max_values(::Type{Bool}) = 2
max_values(::Type{Nothing}) = 1

function union!(s::AbstractSet{T}, itr) where T
haslength(itr) && sizehint!(s, length(s) + length(itr))
Expand Down
21 changes: 21 additions & 0 deletions test/sets.jl
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,27 @@ end
end
end

@testset "optimized union! with max_values" begin
# issue #30315
T = Union{Nothing, Bool}
@test Base.max_values(T) == 3
d = Set{T}()
union!(d, (nothing, true, false))
@test length(d) == 3
@test d == Set((nothing, true, false))
@test nothing in d
@test true in d
@test false in d

for X = (Int8, Int16, Int32, Int64)
@test Base.max_values(Union{Nothing, X}) == (sizeof(X) < sizeof(Int) ?
2^(8*sizeof(X)) + 1 :
typemax(Int))
end
# this does not account for non-empty intersections of the unioned types
@test Base.max_values(Union{Int8,Int16}) == 2^8 + 2^16
end

struct OpenInterval{T}
lower::T
upper::T
Expand Down

2 comments on commit f49cb42

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.