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

Re-enable GC of RGFs which haven't been drop_expr()d #63

Merged
merged 1 commit into from
May 31, 2023

Conversation

c42f
Copy link
Contributor

@c42f c42f commented May 30, 2023

Revert some of #62

Originally GC of RGFs was implemented because people seemed to need it. So dropping support for this completely doesn't seem great.

Instead, this PR lets normal RGF bodies be GC'd as always. But if one calls drop_expr, we upgrade the WeakRef to a normal strong reference in the cache, preventing the body from being GC'd thereafter.

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #63 (74838eb) into master (4ffaeb3) will decrease coverage by 4.38%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   94.00%   89.62%   -4.38%     
==========================================
  Files           1        1              
  Lines         100      106       +6     
==========================================
+ Hits           94       95       +1     
- Misses          6       11       +5     
Impacted Files Coverage Δ
src/RuntimeGeneratedFunctions.jl 89.62% <41.66%> (-4.38%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@c42f c42f force-pushed the c42f/re-enable-gc branch from 74838eb to c8bd814 Compare May 31, 2023 19:29
@c42f
Copy link
Contributor Author

c42f commented May 31, 2023

Ok I think I cleaned those things up now. Should be good to go :)

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.

3 participants