Skip to content

Commit

Permalink
Fix push! for OffsetVectors, add tests for push! and append! on Abstr…
Browse files Browse the repository at this point in the history
…actVector (#55480)

Per
#55470 (comment),
the `push!(::AbstractArray, ...)` array implementation assumed one-based
indexing and did not account for an `OffsetVector`
scenario.

Here we add tests for `push!(::AbstractArray, ...)` and
`append(::AbstractArray, ...)` including using `@invoke` to test the
effect on `OffsetVector`.

cc: @fredrikekre
  • Loading branch information
mkitti authored and KristofferC committed Sep 12, 2024
1 parent ed181e7 commit 4917cb7
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 4 deletions.
7 changes: 4 additions & 3 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3532,24 +3532,25 @@ function push!(a::AbstractVector{T}, item) where T
itemT = item isa T ? item : convert(T, item)::T
new_length = length(a) + 1
resize!(a, new_length)
a[new_length] = itemT
a[end] = itemT
return a
end

# specialize and optimize the single argument case
function push!(a::AbstractVector{Any}, @nospecialize x)
new_length = length(a) + 1
resize!(a, new_length)
a[new_length] = x
a[end] = x
return a
end
function push!(a::AbstractVector{Any}, @nospecialize x...)
@_terminates_locally_meta
na = length(a)
nx = length(x)
resize!(a, na + nx)
e = lastindex(a) - nx
for i = 1:nx
a[na+i] = x[i]
a[e+i] = x[i]
end
return a
end
Expand Down
18 changes: 17 additions & 1 deletion test/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1437,14 +1437,30 @@ using .Main.OffsetArrays
end

@testset "Check push!($a, $args...)" for
a in (["foo", "Bar"], SimpleArray(["foo", "Bar"]), OffsetVector(["foo", "Bar"], 0:1)),
a in (["foo", "Bar"], SimpleArray(["foo", "Bar"]), SimpleArray{Any}(["foo", "Bar"]), OffsetVector(["foo", "Bar"], 0:1)),
args in (("eenie",), ("eenie", "minie"), ("eenie", "minie", "mo"))
orig = copy(a)
push!(a, args...)
@test length(a) == length(orig) + length(args)
@test a[axes(orig,1)] == orig
@test all(a[end-length(args)+1:end] .== args)
end

@testset "Check append!($a, $args)" for
a in (["foo", "Bar"], SimpleArray(["foo", "Bar"]), SimpleArray{Any}(["foo", "Bar"]), OffsetVector(["foo", "Bar"], 0:1)),
args in (("eenie",), ("eenie", "minie"), ("eenie", "minie", "mo"))
orig = copy(a)
append!(a, args)
@test length(a) == length(orig) + length(args)
@test a[axes(orig,1)] == orig
@test all(a[end-length(args)+1:end] .== args)
end

@testset "Check sizehint!($a)" for
a in (["foo", "Bar"], SimpleArray(["foo", "Bar"]), SimpleArray{Any}(["foo", "Bar"]), OffsetVector(["foo", "Bar"], 0:1))
@test sizehint!(a, 10) === a
end

@testset "splatting into hvcat" begin
t = (1, 2)
@test [t...; 3 4] == [1 2; 3 4]
Expand Down
29 changes: 29 additions & 0 deletions test/offsetarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,18 @@ v2 = copy(v)
@test v2[end-1] == 2
@test v2[end] == 1

# push!(v::AbstractVector, x...)
v2 = copy(v)
@test @invoke(push!(v2::AbstractVector, 3)) === v2
@test v2[axes(v,1)] == v
@test v2[end] == 3
@test v2[begin] == v[begin] == v[-2]
v2 = copy(v)
@test @invoke(push!(v2::AbstractVector, 5, 6)) == v2
@test v2[axes(v,1)] == v
@test v2[end-1] == 5
@test v2[end] == 6

# append! from array
v2 = copy(v)
@test append!(v2, [2, 1]) === v2
Expand All @@ -399,6 +411,23 @@ v2 = copy(v)
@test v2[axes(v, 1)] == v
@test v2[lastindex(v)+1:end] == [2, 1]

# append!(::AbstractVector, ...)
# append! from array
v2 = copy(v)
@test @invoke(append!(v2::AbstractVector, [2, 1]::Any)) === v2
@test v2[axes(v, 1)] == v
@test v2[lastindex(v)+1:end] == [2, 1]
# append! from HasLength iterator
v2 = copy(v)
@test @invoke(append!(v2::AbstractVector, (v for v in [2, 1])::Any)) === v2
@test v2[axes(v, 1)] == v
@test v2[lastindex(v)+1:end] == [2, 1]
# append! from SizeUnknown iterator
v2 = copy(v)
@test @invoke(append!(v2::AbstractVector, (v for v in [2, 1] if true)::Any)) === v2
@test v2[axes(v, 1)] == v
@test v2[lastindex(v)+1:end] == [2, 1]

# other functions
v = OffsetArray(v0, (-3,))
@test lastindex(v) == 1
Expand Down

0 comments on commit 4917cb7

Please sign in to comment.