From f37e6c49a1107c7593430b6d01520e6f62a41cb4 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Wed, 7 Dec 2022 09:23:47 +0600 Subject: [PATCH 1/5] add test demonstrating overflow in countsort --- test/sorting.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sorting.jl b/test/sorting.jl index 37bad7d23c94b..f9cb6d91dde15 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -897,6 +897,7 @@ end @testset "Count sort near the edge of its range" begin @test issorted(sort(rand(typemin(Int):typemin(Int)+100, 1000))) @test issorted(sort(rand(typemax(Int)-100:typemax(Int), 1000))) + @test issorted(sort(rand(Int8, 600))) end # This testset is at the end of the file because it is slow. From 2ea650897fb53a20754c14ef9a9952e9fe0a157d Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Wed, 7 Dec 2022 09:26:10 +0600 Subject: [PATCH 2/5] fix overflow in countsort --- base/sort.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/sort.jl b/base/sort.jl index 932da36b9e1d6..e44c671da49cc 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -830,7 +830,7 @@ maybe_reverse(o::ForwardOrdering, x) = x maybe_reverse(o::ReverseOrdering, x) = reverse(x) function _sort!(v::AbstractVector{<:Integer}, ::CountingSort, o::DirectOrdering, kw) @getkw lo hi mn mx scratch - range = o === Reverse ? mn-mx : mx-mn + range = maybe_unsigned(o === Reverse ? mn-mx : mx-mn) offs = 1 - (o === Reverse ? mx : mn) counts = fill(0, range+1) # TODO use scratch (but be aware of type stability) From a7602fb299403397a734c86462dfcdafce595b07 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Wed, 7 Dec 2022 12:43:47 +0600 Subject: [PATCH 3/5] convert back to signed without overflow error --- base/sort.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/sort.jl b/base/sort.jl index e44c671da49cc..ec74bbd29260a 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -843,7 +843,7 @@ function _sort!(v::AbstractVector{<:Integer}, ::CountingSort, o::DirectOrdering, lastidx = idx + counts[i] - 1 val = i-offs for j = idx:lastidx - v[j] = val + v[j] = val isa Unsigned && eltype(v) <: Signed ? signed(val) : val end idx = lastidx + 1 end From 34cad5652d88b64e38d0d89e40f58755763084ac Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Thu, 8 Dec 2022 16:45:17 +0600 Subject: [PATCH 4/5] add another failing test --- test/sorting.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sorting.jl b/test/sorting.jl index f9cb6d91dde15..614946a8cc4f6 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -765,6 +765,7 @@ end @testset "Unions with missing" begin @test issorted(sort(shuffle!(vcat(fill(missing, 10), rand(Int, 100))))) + @test issorted(sort(vcat(rand(Int8, 600), [missing]))) end @testset "Specific algorithms" begin From d8cadea6551b78718442f759aff1d87aa83dc12c Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Thu, 8 Dec 2022 16:46:46 +0600 Subject: [PATCH 5/5] remove unnecessary type annotations (fixes tests) This fixes the test failure because it allows for automatic conversion. The manual for implementing the AbstractArray interface also does not recomend a type signature for the value arg in setindex!. --- base/sort.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index ec74bbd29260a..2dd81829312d0 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -509,12 +509,12 @@ struct WithoutMissingVector{T, U} <: AbstractVector{T} new{nonmissingtype(eltype(data)), typeof(data)}(data) end end -Base.@propagate_inbounds function Base.getindex(v::WithoutMissingVector, i::Integer) +Base.@propagate_inbounds function Base.getindex(v::WithoutMissingVector, i) out = v.data[i] @assert !(out isa Missing) out::eltype(v) end -Base.@propagate_inbounds function Base.setindex!(v::WithoutMissingVector{T}, x::T, i) where T +Base.@propagate_inbounds function Base.setindex!(v::WithoutMissingVector, x, i) v.data[i] = x v end