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

Add eltype methods for Nullable #9281

Merged
merged 1 commit into from
Dec 9, 2014
Merged

Add eltype methods for Nullable #9281

merged 1 commit into from
Dec 9, 2014

Conversation

johnmyleswhite
Copy link
Member

This PR adds eltype methods for Nullable, some stdlib documentation for the non-container methods implemented by Nullable and fixes some small whitespace quirks in the stdlib docs.

@ivarne
Copy link
Sponsor Member

ivarne commented Dec 9, 2014

[off-topic]
Should we document that eltype works for (some, not all) types as well?

It seems to me like it would be better to only define eltype for types and have a two generic fallbacks, to make it work for values too.

eltype(x::DataType) = Any
eltype(x) = eltype(typeof(x))
#edited to add:
# Types who wants eltype to work will then define only one method for the type
eltype{T}(::Type{Nullable{T}}) = T
eltype{T,n}(::Type{AbstractArray{T,n}}) = T
eltype{T<:AbstractArray}(::Type{T}) = eltype(super(T))

@JeffBezanson
Copy link
Sponsor Member

Why would that be better? Working on values is more flexible; otherwise you're forced to represent something's element type in its type.

@JeffBezanson
Copy link
Sponsor Member

Ah, I see I might have misunderstood you: it would be better to have fallbacks that allow you to define eltype only for a type, rather than needing two definitions. Yes that would be good.

@johnmyleswhite
Copy link
Member Author

Are there cases where something's eltype isn't perfectly determined by its type?

@JeffBezanson
Copy link
Sponsor Member

Right now I don't think there are any.

@ivarne
Copy link
Sponsor Member

ivarne commented Dec 9, 2014

I don't think so, but there might be possible to construct some examples where it is useful. Even with my suggestion you can still add eltype definitions to values, so it won't break anything.

@JeffBezanson
Copy link
Sponsor Member

I found a case where eltype is difficult to compute from the type (and in fact our current code is not fully correct). For Combinations and Permutations the best definition would be

eltype(p::Permutations) = typeof(p.a[[1:length(p.a)]])

@johnmyleswhite
Copy link
Member Author

Ok. Should I wait and rebase this PR and after we switch to the fallovers?

@JeffBezanson
Copy link
Sponsor Member

I'll just merge this now, since it will be very easy to fix when we decide what to do with eltype.

JeffBezanson added a commit that referenced this pull request Dec 9, 2014
Add eltype methods for Nullable
@JeffBezanson JeffBezanson merged commit 98865fd into master Dec 9, 2014
@johnmyleswhite
Copy link
Member Author

Thanks, Jeff.

@ivarne ivarne deleted the jmw/eltype branch December 9, 2014 18:14
JeffBezanson added a commit that referenced this pull request Dec 9, 2014
to define it for both types and values

ref discussion in #9281
@StefanKarpinski
Copy link
Sponsor Member

Regarding the permutation type, I've thought that we should maybe have a Permutation type that wraps something indexable and presents a permuted view of it. That would eliminate the copying performance issue that this has and if that types is immutable, would force explicit copying to be done if the user wants to do mutation.

JeffBezanson added a commit that referenced this pull request Feb 9, 2015
to define it for both types and values

ref discussion in #9281
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.

None yet

4 participants