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 leaks #304

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Fix memory leaks #304

merged 1 commit into from
Feb 23, 2023

Conversation

kddnewton
Copy link
Collaborator

This commit fixes a couple of memory leaks in YARP. They include:

  • The LambdaNode type parses its scope using a Scope node, but does not retain a pointer to it. As such, the scope gets allocated but never deallocated. Instead, we now hold onto it and release it when the overall tree is deallocated.
  • When parsing bare hashes as arguments, the first association in the hash was always being dropped. This is now fixed, the tests are fixed, and the memory leak is fixed.
  • When creating a rescue node, the statements was being allocated on its own and then immediately discarded without deallocating. We now manually deallocate it, but this is something that we need to fix in a future PR because there's no reason to create one in the first place.
  • ForNode nodes were parsing their locals with a separate scope, but then discarding the scope. They actually shouldn't parse using a separate scope at all, so all of that is now dropped.

This commit fixes a couple of memory leaks in YARP. They include:

* The LambdaNode type parses its scope using a Scope node, but does
  not retain a pointer to it. As such, the scope gets allocated but
  never deallocated. Instead, we now hold onto it and release it
  when the overall tree is deallocated.
* When parsing bare hashes as arguments, the first association in
  the hash was always being dropped. This is now fixed, the tests
  are fixed, and the memory leak is fixed.
* When creating a rescue node, the statements was being allocated
  on its own and then immediately discarded without deallocating.
  We now manually deallocate it, but this is something that we
  need to fix in a future PR because there's no reason to create
  one in the first place.
* ForNode nodes were parsing their locals with a separate scope,
  but then discarding the scope. They actually shouldn't parse
  using a separate scope at all, so all of that is now dropped.
@kddnewton kddnewton merged commit fd78071 into main Feb 23, 2023
@kddnewton kddnewton deleted the fix-memory branch February 23, 2023 21:15
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.

1 participant