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 with setters #76

Merged
merged 2 commits into from
Sep 22, 2020
Merged

Conversation

blindmonkey
Copy link

This PR aims to fix issue #64, which reports a memory leak when setting a field which stores a reference type.

Unfortunately, I'm not very familiar with raw memory manipulation, so I'm not 100% sure whether this is the correct thing to do in all cases, but it appears to address the issue when I apply this fix in our project.

I wasn't able to run the tests locally, but I'm happy to look into any tests this PR breaks.

@wickwirew
Copy link
Owner

Thanks for looking into this! Glad it was that simple of a fix. Ran everything locally an confirmed the leak is gone. IIRC earlier in Swift doing pointee = value would leak, but initialize(to: value) did not. I'm not sure what changed, or maybe I mixed them up.

This does break a few tests though. When using pointee, it requires that the memory already be initialized. So in the case for constructing a value from the type, that fails. Maybe on the set method you can add initializing: Bool and conditionally use initialize(to:) and pointee. Then set that to true when being used to construct the value in Factory.swift

@blindmonkey
Copy link
Author

Thanks for the feedback, @wickwirew! I added an option to set called initialize, with a default value of false. I was able to get the tests to run (hint: comment out LinuxMain.swift), so I was able to confirm that everything passes.

@wickwirew wickwirew merged commit 044f091 into wickwirew:master Sep 22, 2020
@wickwirew
Copy link
Owner

wickwirew commented Sep 22, 2020

Awesome, thanks again! Pushed a new version 2.2.2

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.

None yet

2 participants