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

fix memory leak when unpacking at top-level scope #11

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

marius311
Copy link
Contributor

The current macro expands @unpack x = foo() to

var### = foo()
begin
    x = unpack(var###, Val{:x})
end
var###

where var### is a new gensymed name each time, but if you ever do @unpack from the REPL / IJulia, then all the var### are in Main forever, thus leaking memory.

This makes it so we expand to

local x
let var### = foo()
    begin
        x = unpack(var###, Val{:x})
    end
    var###
end

where the local is needed to make sure scoping is always right.

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2020

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #11   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           28        28           
=========================================
  Hits            28        28           
Impacted Files Coverage Δ
src/UnPack.jl 100.00% <100.00%> (ø)

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 ce98591...063a9a2. Read the comment docs.

@marius311
Copy link
Contributor Author

Wait, sorry, still an issue in the top-level scope. Fixing...

@marius311
Copy link
Contributor Author

Hmmm crap I can't think of any way to solve this in a way that works both in the global and local scope? Any ideas @mauro3?

@marius311
Copy link
Contributor Author

Ok, got some guidance on discourse and I think its ready for review.

@mauro3
Copy link
Owner

mauro3 commented Aug 14, 2020

Looks good. I thought about requiring a test but I think it would be too fiddly and brittle (searching through the variables returned by names(Main, all=true)).

Thanks to @yuyichao for the help on discourse! I didn't know that trick.

PS: for a bit of history, here the original PR mauro3/Parameters.jl#13 adding @unpack.

@mauro3 mauro3 merged commit a5987f7 into mauro3:master Aug 14, 2020
@marius311
Copy link
Contributor Author

PS: for a bit of history, here the original PR mauro3/Parameters.jl#13 adding @unpack.

Nice. I hope one day we can look back at this whole repo too as a historical anecdote because your proposal here will have been accepted :)

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