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

Make PersistentDict behave like an IdDict #52193

Merged
merged 3 commits into from
Nov 25, 2023

Conversation

vchuravy
Copy link
Member

As noted by @topolarity in #51993 (comment)

julia> x = Int64[0];

julia> d = Base.PersistentDict(x => false, [1] => true);

julia> x[1] = 1

julia> d[x]
true

I was already considering doing hash(sv::ScopedValue) = objectid(sv), but as cody showed this is a general issue,
The current behaviour is consistent with Dict but not with ImmutableDict (which PersistentDict is a close cousin off).

@vchuravy vchuravy added this to the 1.11 milestone Nov 16, 2023
@vchuravy vchuravy added needs decision A decision on this change is needed needs tests Unit tests are required for this change labels Nov 16, 2023
base/hamt.jl Outdated Show resolved Hide resolved
@vchuravy vchuravy marked this pull request as ready for review November 16, 2023 21:27
@vchuravy vchuravy changed the title Make PersistentDict behave like an IdDict for mutable values Make PersistentDict behave like an IdDict Nov 16, 2023
base/hamt.jl Outdated Show resolved Hide resolved
@vchuravy vchuravy removed the needs tests Unit tests are required for this change label Nov 17, 2023
@Keno
Copy link
Member

Keno commented Nov 18, 2023

I think this makes sense, but I do wonder whether we shouldn't parameterize this on what hash/equality you want. I think both ===/objectid and isequal/Base.hash make sense, so it should probably be up to the user (with the ScopedValue use case using ===).

@Keno
Copy link
Member

Keno commented Nov 20, 2023

I'd like to finish #51993 if possible, so why don't we go ahead with this and then we can generalize in the future.

base/dict.jl Outdated Show resolved Hide resolved
base/hamt.jl Outdated Show resolved Hide resolved
base/hamt.jl Outdated Show resolved Hide resolved
@vchuravy vchuravy force-pushed the vc/make_persistent_dict_iddict branch from 4581d0d to e3abaaf Compare November 22, 2023 01:25
@vchuravy vchuravy removed the needs decision A decision on this change is needed label Nov 24, 2023
@vchuravy vchuravy merged commit 9ea29d9 into master Nov 25, 2023
7 checks passed
@vchuravy vchuravy deleted the vc/make_persistent_dict_iddict branch November 25, 2023 15:13
mkitti pushed a commit to mkitti/julia that referenced this pull request Dec 9, 2023
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.

6 participants