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

support multi-arg push! and unshift! #4782

Merged
merged 1 commit into from
Nov 19, 2013
Merged

Conversation

stevengj
Copy link
Member

This patch extends push! and unshift! to accept multiple arguments: push!(A, items...) and unshift!(A, items...), whenever the 1-item versions are defined. For example:

julia> push!([1,2,3], 4,5)
5-element Array{Int64,1}:
 1
 2
 3
 4
 5

julia> unshift!([1,2,3], 4,5)
5-element Array{Int64,1}:
 4
 5
 1
 2
 3

Note that the ordering of the arguments is the same as their ordering in the resulting array (so the items are pushed from left to right, but unshifted from right to left). This seems sensible to me (changing the ordering is a bit surprising), it mirrors append! and prepend!, and e.g. the Javascript push/unshift functions have the same semantics.

By default, these are implemented in terms of the 1-item versions. Specific types could, of course, define specialized multi-argument versions, e.g. push!(A::Array, items...) could be defined to grow the array by several items at once and avoid overhead. But this is an optimization that can be done later if it seems important. The main thing is that this patch does not introduce any performance regressions, it just increases convenience.

@stevengj
Copy link
Member Author

(On a separate note, I feel like there should be fallback versions of append!, prepend!, and empty! for AbstractArray that simply unshift/pop repeatedly ...)

@kmsquire
Copy link
Member

I like it.

I'm curious about the overhead of splatting. Would it be any slower if we removed append! and prepend! entirely, and used this interface instead?

@stevengj
Copy link
Member Author

At the very least, splatting has to make an extra copy of the data to convert it to a tuple, so that can't be good if you have a large array...

@StefanKarpinski
Copy link
Member

I think that append! and prepend! should probably stay. @JeffBezanson should probably have the final word, but I'm fairly certain that he's not going to be pro-splatting.

@stevengj
Copy link
Member Author

Just to be clear, I'm not proposing eliminating prepend and append. I think they have to stay for performance and memory reasons, not to mention convenience. That's not what this patch is about.

@kmsquire
Copy link
Member

@stevengj, I don't think there was any misunderstanding.

More related to this request, this behavior would conflict with @StefanKarpinski's suggestion in #3439 (comment), regarding the potential use of push!(A, idx, item) instead of insert!(A, idx, item)

@JeffBezanson
Copy link
Member

I'm fine with this change, but I think it would be better for implementers of push! to handle multiple arguments, rather than implementing it on top of the 1-argument version. That leads to a more efficient implementation where the array is grown only once, knowing how many arguments were passed.

@stevengj
Copy link
Member Author

@JeffBezanson, I agree, but I figured that kind of optimization could be done later; this version is no worse than what we have now.

JeffBezanson added a commit that referenced this pull request Nov 19, 2013
support multi-arg push! and unshift!
@JeffBezanson JeffBezanson merged commit 0e5f95b into JuliaLang:master Nov 19, 2013
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