Skip to content

Correctly free property and method lists #670

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

Merged

Conversation

TitanNano
Copy link
Contributor

While working on the patch for #554 I noticed that both free_property_list_func and free_method_list_func are not correctly freeing their data.

Property List

free_property_list_func still uses the ScriptInstance trait to obtain the list length. This is incorrect. It should take the length of the Vec which is being removed from the HashMap.

Method List

Has the same issue as the property list but, moreover, it is also never dropping the Vec inside the HashMap. So this memory is effectively leaked.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-670

@Bromeon Bromeon added bug c: engine Godot classes (nodes, resources, ...) labels Apr 22, 2024
@Bromeon Bromeon force-pushed the jovan/script_instance_list_free branch from 6307996 to d070dfe Compare April 22, 2024 20:04
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@TitanNano TitanNano force-pushed the jovan/script_instance_list_free branch from d070dfe to b14f4d1 Compare April 23, 2024 19:11
@Bromeon Bromeon added this pull request to the merge queue Apr 23, 2024
@Bromeon
Copy link
Member

Bromeon commented Apr 23, 2024

Thanks 🙂

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2024
@Bromeon Bromeon added this pull request to the merge queue Apr 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2024
@Bromeon
Copy link
Member

Bromeon commented Apr 23, 2024

Uh, CI again... will need to see if this is an issue that's also happening with other branches...

@Bromeon Bromeon added this pull request to the merge queue Apr 25, 2024
Merged via the queue into godot-rust:master with commit 2e14deb Apr 25, 2024
16 checks passed
@TitanNano TitanNano deleted the jovan/script_instance_list_free branch April 25, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: engine Godot classes (nodes, resources, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants