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

Implemented WeakKeyIdDict #402

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mauro3
Copy link

@mauro3 mauro3 commented Jul 20, 2018

This is what I put together in JuliaLang/julia#28161 (closed PR). I thought it could perhaps live here. Note that a WeakKeyIdDict might go into Base sometime in 1.x: JuliaLang/julia#28182, but who knows when.

@codecov
Copy link

codecov bot commented Jul 20, 2018

Codecov Report

Merging #402 into master will decrease coverage by 0.08%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
- Coverage   98.65%   98.57%   -0.09%     
==========================================
  Files          30       31       +1     
  Lines        1716     1760      +44     
==========================================
+ Hits         1693     1735      +42     
- Misses         23       25       +2
Impacted Files Coverage Δ
src/weakkeyid_dict.jl 95.45% <95.45%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6ee8e0...ccc7543. Read the comment docs.

@mauro3
Copy link
Author

mauro3 commented Sep 24, 2018

Is there any interest in this? Otherwise please close.

@thautwarm
Copy link

Any news?

@mauro3
Copy link
Author

mauro3 commented Dec 21, 2022

After the great recent interest in this PR (two people in two month! ;-), I updated it such that it passes again tests and fixed conflicts. Ready to go, as far as I'm concerned.

@thautwarm
Copy link

Thanks. This is quite useful!

@oxinabox
Copy link
Member

Per #479
we are not accepting new data structures til post-1.0.0

I guess a case could be made that this PR was open long before then and so should be an exception?

@ericphanson
Copy link
Member

+1 for an exception, since I want to use this too :). Or maybe it can be it's own small package?

Base.get(wkh::WeakKeyIdDict{K}, key, default) where {K} = lock(() -> get(wkh.ht, WeakRefForWeakDict(key), default), wkh)
Base.get(default::Base.Callable, wkh::WeakKeyIdDict{K}, key) where {K} = lock(() -> get(default, wkh.ht, WeakRefForWeakDict(key)), wkh)
Base.get!(wkh::WeakKeyIdDict{K}, key, default) where {K} = lock(() -> get!(wkh.ht, WeakRefForWeakDict(key), default), wkh)
Base.get!(default::Base.Callable, wkh::WeakKeyIdDict{K}, key) where {K} = lock(() -> get!(default, wkh.ht, WeakRefForWeakDict(key)), wkh)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to use Base.@lock here instead of the closure form. It can be more performant and keeps stacktraces shorter; I have a recursive call where this almost doubles the number of stack frames. It is exported on Julia 1.9 and present in many versions of Julia, but not present in 1.0 unfortunately, which DataStructures still supports.

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.

4 participants