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.

Sign up for GitHub

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

Jump to bottom

Use GDExtension to_string in Node #92827

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Use GDExtension to_string in Node #92827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,7 @@ void Object::notification(int p_notification, bool p_reversed) {
}

String Object::to_string() {
// Keep this method in sync with `Node::to_string`.
if (script_instance) {
bool valid;
String ret = script_instance->to_string(&valid);
Expand Down
8 changes: 7 additions & 1 deletion scene/main/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2583,6 +2583,7 @@ void Node::get_storable_properties(HashSet<StringName> &r_storable_properties) c
}

String Node::to_string() {
// Keep this method in sync with `Object::to_string`.
ERR_THREAD_GUARD_V(String());
if (get_script_instance()) {
bool valid;
Expand All @@ -2591,7 +2592,12 @@ String Node::to_string() {
return ret;
}
}

if (_get_extension() && _get_extension()->to_string) {
String ret;
GDExtensionBool is_valid;
_get_extension()->to_string(_get_extension_instance(), &is_valid, &ret);
return ret;
}
Comment on lines +2595 to +2600
Copy link
Contributor

Choose a reason for hiding this comment

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

is_valid has neither a defined input value (uninitialized), nor is its output checked -- is that intended?

Copy link
Contributor

@dsnopek dsnopek Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Those are good questions! However, this code exactly matches what we already have in Object::to_string(), so it isn't making things any worse. :-) If we do end up evaluating how is_valid should be used, then it should be fixed in both places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarification!

Since we'll likely forget to update one of the two places in that case, should we add a comment in both? (Or directly reuse a function, not sure how easy it would be).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just copied Object::to_string but I also thought it was weird that is_valid is unused. We should probably check before returning, like we do with valid in scripts:

godot/core/object/object.cpp

Lines 926 to 932 in e96ad5a

if (script_instance) {
bool valid;
String ret = script_instance->to_string(&valid);
if (valid) {
return ret;
}
}

Note that valid is also uninitialized. I guess this variable is always assigned by to_string so there's no need to initialize it, but I'm not too familiar with C++ so if we should always initialize it just let me know.

Copy link
Member

Choose a reason for hiding this comment

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

At least the default implementations of to_string() seem to properly set r_valid to false before errors, and true on success.

That's a common pattern for calling methods on ScriptInstance or GDExtension, though it's hard to guarantee that it's always safe. The tradeoff between avoiding one potentially unneeded initialization and risking using uninitialized memory needs to be assessed (especially whether not initializing these actually makes a difference to performance at all).

It's a much bigger scope than just this PR though. For this I'd suggest just putting a comment in both implementations that they should be kept in sync (not for what to do with valid specifically, but the whole method).

return (get_name() ? String(get_name()) + ":" : "") + Object::to_string();
}

Expand Down
Loading