Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add push! implementation for AbstractArray depending only on resize! #55470

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Add push! implementation for AbstractArray depending only on resize! #55470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3525,13 +3525,45 @@ julia> map(+, [1 2; 3 4], [1,10,100,1000], zeros(3,1)) # iterates until 3rd is
"""
map(f, it, iters...) = collect(Generator(f, it, iters...))

# Generic versions of push! for AbstractVector
# These are specialized further for Vector for faster resizing and setindexing
function push!(a::AbstractVector{T}, item) where T
# convert first so we don't grow the array if the assignment won't work
itemT = item isa T ? item : convert(T, item)::T
new_length = length(a) + 1
resize!(a, new_length)
a[new_length] = itemT
Copy link
Member

Choose a reason for hiding this comment

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

julia> o = OffsetVector([1, 2, 3], -1);

julia> @invoke push!(o::AbstractVector, 4::Any)
ERROR: BoundsError: attempt to access 4-element OffsetArray(::Vector{Int64}, 0:3) with eltype Int64 with indices 0:3 at index [4]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this re-establishes the state before #51903 but I agree this is an issue.

Would end work better here or should we require or dispatch on one-based indexing?

Copy link
Contributor

Choose a reason for hiding this comment

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

on the specific case of OffsetArrays, resize! works on the the parent array (so length(a) + 1 is fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand your point.

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
return a
end
function push!(a::AbstractVector{Any}, @nospecialize x...)
@_terminates_locally_meta
na = length(a)
nx = length(x)
resize!(a, na + nx)
for i = 1:nx
a[na+i] = x[i]
end
return a
end
vtjnash marked this conversation as resolved.
Show resolved Hide resolved

# multi-item push!, pushfirst! (built on top of type-specific 1-item version)
# (note: must not cause a dispatch loop when 1-item case is not defined)
push!(A, a, b) = push!(push!(A, a), b)
push!(A, a, b, c...) = push!(push!(A, a, b), c...)
pushfirst!(A, a, b) = pushfirst!(pushfirst!(A, b), a)
pushfirst!(A, a, b, c...) = pushfirst!(pushfirst!(A, c...), a, b)

# sizehint! does not nothing by default
sizehint!(a::AbstractVector, _) = a
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change? Also no tests.

Copy link
Contributor Author

@mkitti mkitti Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#51903 introduced a dependency on sizehint! for append! that did not previously exist.

I will add tests for append.


## hashing AbstractArray ##

const hash_abstractarray_seed = UInt === UInt64 ? 0x7e2d6fb6448beb77 : 0xd4514ce5
Expand Down
9 changes: 9 additions & 0 deletions test/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,15 @@ using .Main.OffsetArrays
end
end

@testset "Check push!($a, $args...)" for
a in (["foo", "Bar"], SimpleArray(["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 all(a[end-length(args)+1:end] .== args)
end

@testset "splatting into hvcat" begin
t = (1, 2)
@test [t...; 3 4] == [1 2; 3 4]
Expand Down