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

Jump to bottom

abstractarray: fix append!(::AbstractVector, ...) interface #49754

Merged
merged 1 commit into from
May 12, 2023
Merged

abstractarray: fix append!(::AbstractVector, ...) interface #49754

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
19 changes: 12 additions & 7 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,6 +1167,8 @@ See [`sizehint!`](@ref) for notes about the performance model.
See also [`vcat`](@ref) for vectors, [`union!`](@ref) for sets,
and [`prepend!`](@ref) and [`pushfirst!`](@ref) for the opposite order.
"""
function append! end

function append!(a::Vector, items::AbstractVector)
itemindices = eachindex(items)
n = length(itemindices)
Expand All @@ -1180,18 +1182,21 @@ push!(a::AbstractVector, iter...) = append!(a, iter)

append!(a::AbstractVector, iter...) = foldl(append!, iter, init=a)

function _append!(a, ::Union{HasLength,HasShape}, iter)
function _append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I wonder if we should move these generic definitions into base/abstractarray.jl and define simple definition of append! for Vector-inputs here (same for prepend!). Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Special casing a concrete implementation should IMO always be preferrable over assuming more of a generic version. It's philosophically the same reason why @inbounds in a function accepting AbstractArray is bad - there's no guarantee that the concrete type isn't doing any funny business in indexing; the local information cannot be enough to ensure the use is safe.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

A potential drawback on the design is that there will be multiple potential method matches in type-unstable code for those functions, that may lead to worse inlining and optimization.

Well, we may want to accept that since the type unstable code will be slow anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think prioritizing correctness over potentially broken fallbacks is a better bet any day of the week.

@_terminates_locally_meta
n = length(a)
i = lastindex(a)
resize!(a, n+Int(length(iter))::Int)
@_safeindex for (i, item) in zip(i+1:lastindex(a), iter)
a[i] = item
for (i, item) in zip(i+1:lastindex(a), iter)
if isa(a, Vector) # give better effects for builtin vectors
@_safeindex a[i] = item
else
a[i] = item
end
end
a
end

function _append!(a, ::IteratorSize, iter)
function _append!(a::AbstractVector, ::IteratorSize, iter)
for item in iter
push!(a, item)
end
Expand Down Expand Up @@ -1246,7 +1251,7 @@ pushfirst!(a::Vector, iter...) = prepend!(a, iter)

prepend!(a::AbstractVector, iter...) = foldr((v, a) -> prepend!(a, v), iter, init=a)

function _prepend!(a, ::Union{HasLength,HasShape}, iter)
function _prepend!(a::Vector, ::Union{HasLength,HasShape}, iter)
@_terminates_locally_meta
require_one_based_indexing(a)
n = length(iter)
Expand All @@ -1257,7 +1262,7 @@ function _prepend!(a, ::Union{HasLength,HasShape}, iter)
end
a
end
function _prepend!(a, ::IteratorSize, iter)
function _prepend!(a::Vector, ::IteratorSize, iter)
n = 0
for item in iter
n += 1
Expand Down