Fix issue 19534 #19566
Fix issue 19534 #19566
Conversation
| @@ -1,22 +1,78 @@ | |||
| # weak key dictionaries | |||
|
|
|||
| type WeakKeyDict{K,V} <: Associative{K,V} | |||
| ht::Dict{Any,V} | |||
| ht::Dict{WeakRef,V} | |||
There was a problem hiding this comment.
This change is unrelated. I can revert it but it seems to me that the stricter type is a bit saver.
| end | ||
| WeakKeyDict() = WeakKeyDict{Any,Any}() | ||
|
|
||
| WeakKeyDict(kv::Tuple{}) = WeakKeyDict() |
There was a problem hiding this comment.
These outer constructors are copies of the Dict ones. It could be DRYed up using something along the lines of
type A1{T}
a::T
A1(x) = new(x)
end
type A2{T}
a::T
A2(x) = new(x)
end
typealias A Union{A1,A2}
(::Type{TA}){TA<:A,T}(x::T) = TA{T}(x)
However, I suspect that there will be boot-strap isssues as WeakKeyDict comes later, thus these constructors would need to be added to weakkeydict.jl. Alternatively, I could put a comment into dict.jl to state to keep the constructors synchronized.
There was a problem hiding this comment.
I went with the comment in dict.jl
|
Could you make the commit message a bit more informative? |
|
@StefanKarpinski, yes will do. I'll also squash into one. (I'll wait for a bit for other comments) |
mauro3
force-pushed
the
m3/issue-19534
branch
from
f12c93d
to
68493e1
Compare
|
Fixed up commit message. Fixed |
| @@ -628,7 +639,10 @@ Dict(1 => rand(2,3), 'c' => "asdf") # just make sure this does not trigger a dep | |||
| @test !isempty(wkd) | |||
|
|
|||
| wkd = empty!(wkd) | |||
| @test wkd==similar(wkd) | |||
| @test typeof(wkd)==typeof(similar(wkd)) | |||
There was a problem hiding this comment.
spaces around == will be more readable. also no need for the blank line before end
There was a problem hiding this comment.
Sorry to have wasted your time, I forgot to push...
|
The Appveyor build got to |
|
That appveyor build is stuck for sure. |
- constructors for Pairs and iterables - updated tests - unrelated: changed wrapped dict to a tighter type
|
Updated formatting of tests according to @tkelman's comment. |
|
If no other opinions here I'll merge this and #19515 later today. |
This fixes #19534.