RFC/WIP: Added IdDict to collection benchmarks #171
Conversation
|
BaseBenchmarks needs to support 0.6 until we're certain there won't be any further 0.6 releases. While it's unlikely, it is the current release version, so we can't be sure of that yet. What I would suggest is to add |
|
Thanks for the speedy feedback! It's not possible to add the needed features of IdDict to Compat.jl (its type parameters, see comment #150 (comment)). My idea was to just not run IdDict tests on 0.6. If that is not an option, would it be ok to let this PR sit until 0.6 is dropped? Or we could close it and I'll try to remember to re-open later. Let me know what you think is best. |
|
Ah, okay. In that case, I'd be fine with letting this sit until 0.6 is dropped, or just putting all of the relevant stuff here behind an appropriate version guard thereby disabling it on 0.6. I'd be fine either way. 🙂 |
mauro3
force-pushed
the
m3/IdDict
branch
2 times, most recently
from
004352e
to
71583ff
Compare
|
Ok, updated with version guards. Maybe worth waiting for the verdict on JuliaLang/Compat.jl#464. |
| @@ -41,24 +48,27 @@ initcoll(C, T) = let initmap = Dict(Vector => Vector, | |||
| Dict => Pair, | |||
| Set => Vector, | |||
| BitSet => Vector) | |||
| IDD && (initmap[IdDict] = Pair) | |||
There was a problem hiding this comment.
As IdDict is here to stay, why not put directly IdDict => Pair in the initialization of initmap? As you defined trivially IdDict anyway on 0.6, it won't be an error, it will just not be used, and this is less code to update when version 0.6 support is dropped. Or am I missing something?
| T in (Int, String, Any) | ||
| coll[C, T] = C(initcoll(C, T)) | ||
| end | ||
|
|
||
| @inline newcoll(::Type{C}, ::Type{T}) where {C,T} = C{T}() | ||
| @inline newcoll(::Type{Dict}, ::Type{T}) where {T} = Dict{T,T}() | ||
| IDD && (@inline newcoll(::Type{IdDict}, ::Type{T}) where {T} = IdDict{T,T}()) |
There was a problem hiding this comment.
Same remark as above, maybe the guard is unnecessary here? (idem for askey below).
|
Adressed @rfourquet comments. I think this could be merged after CI passes (If Compat does something about IdDict (JuliaLang/Compat.jl#464), then it's a one-line update.) |
Adds the new
IdDictto the collection benchmarks, x-ref: JuliaLang/julia#25210IdDict is missing the
pop!(iddict)method, thus a few more guards. Probably this method should be added toIdDict.TODO: this should probably be made compatible with older Julia versions.
CC: @rfourquet