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.

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

Make append! and push! definitions more generic #32065

Merged
merged 1 commit into from
May 21, 2019
Merged

Conversation

nalimilan
Copy link
Member

Some methods for append! and push! for Array actually work for any AbstractArray since they only use resize!, setindex! and push!. Broaden the signatures so that custom array types do not need to copy definitions from Base.

BitArray uses special methods, one of which was apparently not covered by tests.

Ideally, fallback append!(::AbstractVector, ::AbstractVector) and push!(::AbstractVector, ::Any) methods would also be provided. Unfortunately, the existing definitions for Vector call _grow_end instead of resize!, which means that simply broadening the signature to AbstractVector won't work. It would be easy to fix that (using x isa Vector or a helper function), but since the non-Vector code path would not be used in Base I'm not sure that's OK. Comments?

Some methods for append! and push! for Array actually work for any AbstractArray
since they only use resize!, setindex! and push!. Broaden the signatures
so that custom array types do not need to copy definitions from Base.

BitArray uses special methods, one of which was apparently not covered by tests.
@timholy
Copy link
Member

timholy commented May 18, 2019

Should the necessary methods be added to the interfaces chapter? Perhaps on a section on growable arrays?

It would be easy to fix that (using x isa Vector or a helper function), but since the non-Vector code path would not be used in Base I'm not sure that's OK. Comments?

There is a precedent: Base doesn't export an OffsetArray type yet we have a bunch of tests that exercise that functionality.

@nalimilan
Copy link
Member Author

Should the necessary methods be added to the interfaces chapter? Perhaps on a section on growable arrays?

Yes, that would be useful. There are other similar functions to list (like insert!, splice!, prepend! and pop!). Why not just add a new section in the existing table about the AbstractArray interface? It already has sections.

But I'd first want to tackle the question of whether we can provide generic implementations for these.

There is a precedent: Base doesn't export an OffsetArray type yet we have a bunch of tests that exercise that functionality.

So basically you say it would be OK to replace _growend!(a, n) with a isa Vector ? _growend!(a, n) : resize!(a, length(a)+n) even if that's not used directly by Base?

@JeffBezanson
Copy link
Member

It's not clear to me whether the method for append!(::Vector, ::AbstractVector) is even any better than the method for append!(::AbstractVector, ::Any).

@JeffBezanson JeffBezanson merged commit 62a0a3f into master May 21, 2019
@JeffBezanson JeffBezanson deleted the nl/append! branch May 21, 2019 19:50
@nalimilan
Copy link
Member Author

Ah, indeed, so maybe this PR is all we need actually. Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants