Add WeakKeyIdDict #1419
Add WeakKeyIdDict #1419
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1419 +/- ##
==========================================
- Coverage 85.62% 84.58% -1.05%
==========================================
Files 107 109 +2
Lines 28760 29345 +585
==========================================
+ Hits 24627 24822 +195
- Misses 4133 4523 +390
... and 21 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Looks good. Do you want to bump up that coverage (at the moment it is at 50%)? |
fingolfin
force-pushed
the
mh/WeakKeyIdDict
branch
2 times, most recently
from
f32190f
to
b9fbd1b
Compare
|
Good point -- I only copied the existing tests, but of course syncing with master. So I've added some tests, and of course this uncovered various bugs 😂 . I've also tried to add a test for the GC collecting the weak refs, but I can't get it to reliably work, so it is for now disabled. I'll wait for the updated coverage report to see where else I can add some more. |
fingolfin
force-pushed
the
mh/WeakKeyIdDict
branch
from
b9fbd1b
to
86dd7f1
Compare
This should fix various GC issues, and ensure feature parity
fingolfin
force-pushed
the
mh/WeakKeyIdDict
branch
3 times, most recently
from
8b4146d
to
5656ea9
Compare
test/WeakKeyIdDict-test.jl
Outdated
| # The following tests involve the garbage collector and | ||
| # don't work if they are inside a @testset | ||
| wkd = WeakKeyIdDict([42]=>2, [43]=>3, [44]=>4) | ||
| for k in keys(wkd) | ||
| delete!(wkd, k) | ||
| end | ||
| @test isempty(wkd) | ||
| GC.gc() # try to get it to evict some weak references | ||
| @test isempty(wkd) | ||
|
|
||
| # verify that garbage collection takes care of our weak references | ||
| A = [1] | ||
| wkd = WeakKeyIdDict(A => 1) | ||
| let tmp = [ 42 ] | ||
| @test length(wkd) == 1 | ||
| wkd[tmp] = 2 | ||
| @test length(wkd) == 2 | ||
| end | ||
| # at this point there is no strong reference left to the vector [42] | ||
| # previously reachable via tmp | ||
| GC.gc(true) | ||
|
|
||
| @test length(wkd) == 1 | ||
| @test length(keys(wkd)) == 1 | ||
| @test WeakKeyIdDict(A => 1) == wkd |
There was a problem hiding this comment.
Assuming the CI tests pass now, I am fine with how high the coverage now is (> 90%). Note this ugliness: I could not find a way to get those tests which interact with the GC to work "right" while inside a @testset. But I am not aware of a serious drawback of the above, other than it not being reported in the output of Pkg.test("AbstractAlgbera") ... ?
There was a problem hiding this comment.
Does it work if you put it into a let scope? Currently, CI fails as there are two global objects both named A in the tests.
fingolfin
force-pushed
the
mh/WeakKeyIdDict
branch
from
5656ea9
to
bd2d2df
Compare
fingolfin
force-pushed
the
mh/WeakKeyIdDict
branch
from
bd2d2df
to
f33f5de
Compare
|
I wonder if WeakKeyIdDict could do in a separate WeakKeyIdDicts.jl package, so it could be reused elsewhere easier? |
|
Feel free to create such a package, and also to copy the code from here (it is open source after all, and heavily based on the Julia stdlib code anyway, as well as code written by others). |
|
BTW @haberdashPI has created this package here: https://github.com/beacon-biosignals/WeakKeyIdDicts.jl |
See oscar-system/Oscar.jl#1872
CC @mauro3