Fix container and string leaks #490
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some functions return a new instance of such containers, but instead we made a copy of them, without taking ownership of the original created by the function. Now we use a specific constructor taking ownership on the godot_* struct.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This continues what was found in #355 and #356
Some functions return a new instance of
Array,Dictionary,Stringor pool arrays, but instead we made a copy of them, without taking ownership of the original created by the function. Now we use a specific constructor taking ownership on the godot_* struct.Leaks causing error logs on exit still occurred in
Dictionary::values(),Array::duplicate()and other similar functions. But also, some leaks were invisible, such as strings or primitive data, with functions likeDictionary::to_json. I was able to confirm that by calling them in an infinite loop.I havent extensively tested this PR, only tested a few, and then applied the pattern to all similar places I thought were relevant.
Might fix #470
Also #338 (though it might have been fixed by one of the earlier PRs)
Some leaks reported in #223 might have been caused indirectly by the same issue, otherwise it has no clear reproduction.
Tested on #144 on my way, works fine too.