Skip to content

Conversation

@gongpha
Copy link
Contributor

@gongpha gongpha commented Jun 17, 2025

Minor fix on the theme editor preview.

Fix a rarely-happening minor memory leak from not freeing the instantiated node inside the if clause that checks if a node is Control. Also added more error messages

@AThousandShips AThousandShips requested a review from a team June 17, 2025 13:33
@AThousandShips AThousandShips added this to the 4.5 milestone Jun 17, 2025

Node *instance = loaded_scene->instantiate();
if (!instance || !Object::cast_to<Control>(instance)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Useless newline

Copy link
Member

@akien-mga akien-mga Jun 17, 2025

Choose a reason for hiding this comment

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

I think it's fine either way here, but yeah it could be removed as the pattern:

Object *obj = get_some_object();
if (!obj) {

is pretty common in Godot.

But here there's also a later if check that uses instance and so it can make sense indeed to have them separate:

Node *instance;

if (!instance) {}

if (other thing with instance) {}
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Repiteo Repiteo merged commit 3f99c09 into godotengine:master Jun 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 18, 2025

Thanks!

@gongpha gongpha deleted the potentially-memory-leak branch June 19, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

5 participants