-
-
Notifications
You must be signed in to change notification settings - Fork 613
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
Unexpose UtilityFunctions::is_instance_valid()
#1513
Unexpose UtilityFunctions::is_instance_valid()
#1513
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.
LGTM. Makes sense to unexpose if the usage of the function will be breaking something down the line anyways. This should definitely be highlighted in the next release notes with a warning though to make people aware
So many change in downstream extensions.. |
@zhwei820 It's broken. See the bottom here: #1390 (comment) |
@dsnopek It seems that For example, the following dummy GDExtension library function and GDScript will produce a crash when supplying a freed Object: // Must validate supplied object
godot_error MyLib::dummy(const Variant v) {
if (v.get_type() != Variant::OBJECT) return ERR_CANT_CREATE;
auto o = v.operator Object*(); // Crashes here if object was previously freed
if (o == NULL) return ERR_CANT_CREATE;
auto id = o->get_instance_id();
auto x = ObjectDB::get_instance(id);
if (x == NULL) return ERR_CANT_CREATE;
return OK;
} var o = Object.new()
var lib = MyLib.new()
assert(lib.dummy(o) == OK) # Expect object to be valid
o.free()
assert(lib.dummy(o) != OK) # Expect object to be invalid Both assertion cases were previously correctly handled by In a GDExtension context without |
Fixes #1390
The
is_instance_valid()
function doesn't work as developers expect, and unless used very carefully, will cause crashes. Instead, developers should useObjectDB::get_instance()
with object ids to ensure that an instance is still valid.This PR unexposes
is_instance_valid()
so that folks aren't tempted to use it.I'm unsure if we should cherry-pick this or not. Technically it breaks source compatibility, but if developers are using this function, they probably have a bug that's waiting to be exposed. :-)
Note: Bindings aside from godot-cpp may be able to expose
is_instance_valid()
in a non-problematic way. However, since we're trying to match the Godot API and allow raw pointers to objects, we are stuck with this issue. If were able to disallow raw object pointers and require developers to use some kind of "object smart pointer object", that would be a different story.