-
Notifications
You must be signed in to change notification settings - Fork 9
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
What does it mean to have a Platform as a key in a Dict #15
Comments
I'm not sure I follow. Assuming that julia> using Base.BinaryPlatforms
julia> valid_wrappers = Dict{Platform,String}()
Dict{Platform, String}()
julia> p = Platform("x86_64", "windows"; cuda = "10.1");
julia> q = Platform("x86_64", "windows"; cuda = "10.1");
julia> valid_wrappers[p] = "foo"
"foo"
julia> valid_wrappers
Dict{Platform, String} with 1 entry:
Platform(Dict("arch"=>"x86_64", "os"=>"windows", "cuda"=>"10.1"), Dict{String, Function}()) => "foo"
julia> valid_wrappers[q] = "bar"
"bar"
julia> valid_wrappers
Dict{Platform, String} with 1 entry:
Platform(Dict("arch"=>"x86_64", "os"=>"windows", "cuda"=>"10.1"), Dict{String, Function}()) => "bar"
This is in a |
Yeah, I see the issue. On the latest We may want instead to key the dictionary by |
Note that while the serialization captures most things it doesn't capture comparison strategies, but that's fine, since we don't care about those here at all. |
Yeah, that was pretty much my point. It seems good to either be explicit that the id of the object matters (by using an |
Ah, I didn't think of defining |
Can this be closed since JuliaLang/julia#37734 was merged? |
I guess, imo, it should still maybe be an |
The part here:
JLLWrappers.jl/src/toplevel_generators.jl
Line 125 in bc3b6af
uses a
Platform
as a key in a dictionary. But aPlatform
is an object that AFAIU will useobjectid
as part of its hash computation, so for example:It's also a bit unclear to me what happens when you serialize such a dict. Is it still valid when you read it back? Would an
IdDict
be more suitable here to emphasize that the key is based on the constructed object itself and not the content?The text was updated successfully, but these errors were encountered: