-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Use GDExtension to_string
in Node
#92827
Conversation
There was a problem hiding this 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!
if (_get_extension() && _get_extension()->to_string) { | ||
String ret; | ||
GDExtensionBool is_valid; | ||
_get_extension()->to_string(_get_extension_instance(), &is_valid, &ret); | ||
return ret; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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).
Matches the `Object::to_string` implementation.
d2751df
to
29bf60c
Compare
Thanks! |
Before this PR, when a GDExtension class that derives from
Node
implements_to_string
like this:Godot will print
@MyNode@2:MyClass
instead of justMyClass
like it does for otherObject
-derived classes.Matches the
Object::to_string
implementation:godot/core/object/object.cpp
Lines 925 to 940 in e96ad5a