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

Use promote_typejoin() to choose type parameters in Dict constructor #25805

Merged
merged 2 commits into from
Feb 14, 2018

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Jan 29, 2018

This is consistent with Array, NamedTuple and Tuple (via its eltype method), and therefore with the Dict(::AbstractArray{<:Pair}) constructor.

Fixes #25551.

This is consistent with Array, NamedTuple and Tuple (via its eltype method),
and in particular the Dict(::AbstractArray{<:Pair}) constructor.
@nalimilan nalimilan added the missing data Base.missing and related functionality label Jan 29, 2018
Dict(ps::(Pair{K,V} where K)...) where {V} = Dict{Any,V}(ps)
Dict(ps::Pair...) = Dict{Any,Any}(ps)
Dict(ps::Pair{K,V}...) where {K,V} = Dict{K,V}(ps)
Dict(ps::Pair...) = Dict(ps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have one question - is the first method is needed only for performance reasons or covers some special case?

Copy link
Member Author

@nalimilan nalimilan Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's was there for performance, but that's not clear to me...

EDIT: FWIW, these constructors are very old, they have been introduced in 2d24d01 when Dict was called HashTable and before Pair existed.

@nalimilan
Copy link
Member Author

I'll merge in a few days if nobody objects.

Without this Dict(1=>missing, 2=>2) returns a Dict{Int,Any}.
@nalimilan nalimilan merged commit 4c4aaaf into master Feb 14, 2018
@nalimilan nalimilan deleted the nl/dict branch February 14, 2018 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing data Base.missing and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants