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

allow Array{T,N}() for arbitrary N #20250

Closed
wants to merge 2 commits into from
Closed

allow Array{T,N}() for arbitrary N #20250

wants to merge 2 commits into from

Conversation

KristofferC
Copy link
Sponsor Member

Closes #14201

@stevengj
Copy link
Member

This syntax seems a bit weird to me.

@KristofferC
Copy link
Sponsor Member Author

What syntax? Array{T, N}() ?

@martinholters
Copy link
Member

I'd favor deprecating the N==1 and N==2 cases, but if we're going with this, I think we can safely remove those from boot.jl, no?

@KristofferC
Copy link
Sponsor Member Author

Unless they are used there? I was also worried if the splatting carried some performance problems but could just check the generated code. I don't think this should be deprecated, it is a convenient way to put a place holder object in a type that you will swap out later.

@martinholters
Copy link
Member

Unless they are used there?

I think base only uses this for N==1 and only in files included after array.jl, so things should be good.

it is a convenient way to put a place holder object in a type that you will swap out later.

I guess you only need it if N is largish or unknown at code-writing-time, so that spelling out the 0s is impractical/impossible. Is that common? Should we consider different syntax, e.g. empty(Array{T,N})?

@KristofferC
Copy link
Sponsor Member Author

Removed the boot.jl definitions.

@stevengj
Copy link
Member

stevengj commented Jan 27, 2017

I would rather deprecate the N=1 and N=2 cases than add this syntax; I didn't even realize we had those.

@mbauman
Copy link
Sponsor Member

mbauman commented Jan 27, 2017

See #11154 for a bit of history. I think that Vector() and Vector{Int}() are somewhat common first attempts at constructing a vector for new users… but at the same time, learning those signatures generalizes poorly. See the comment within that thread about Array{Any}()'s surprising behavior within that context and the immediate desire for Vector(1,2,3) => Any[1,2,3].

I'm rather ambivalent here, but I agree it should be all or none. It'd be interesting to see how common Vector{T}() is across registered packages.

@KristofferC
Copy link
Sponsor Member Author

Rekt by #20330

@KristofferC KristofferC closed this Feb 2, 2017
@StefanKarpinski
Copy link
Sponsor Member

Sorry, man!

@KristofferC
Copy link
Sponsor Member Author

I just wanted to close #14201 (which I just did) so end result is the same

@tkelman tkelman deleted the kc/array_const branch February 11, 2017 16:57
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.

5 participants