abstractarray: fix append!(::AbstractVector, ...) interface
#49754
Conversation
#47154 mistakenly added `@_safeindex` macro on the `_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)` method, although `@_safeindex` is only valid for builtin vectors i.e. `Vector`. This commit adds `isa` check so that `@_safeindex` is only applied to builtin vectors. The `isa` check should be removed at compile time, so it should not affect the runtime performance. closes #49748
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think prioritizing correctness over potentially broken fallbacks is a better bet any day of the week.
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
Going to merge this. I will refactor the code as suggested above in a separate PR. |
|
Since I looked at this code again for a different reason and noticed this, how did the So just removing the branch entirely ought to be correct! |
|
No, the specialized |
|
Ah, of course 🤦 |
#47154 mistakenly added
@_safeindexmacro on the_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)method, although@_safeindexis only valid for builtin vectors i.e.Vector.This commit adds
isacheck so that@_safeindexis only applied to builtin vectors. Theisacheck should be removed at compile time, so it should not affect the runtime performance.closes #49748