Skip to content

GDExtension: Provide free_property_list_func with length of array #91179

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

Merged
merged 1 commit into from
Apr 29, 2024
Merged

GDExtension: Provide free_property_list_func with length of array #91179

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
16 changes: 11 additions & 5 deletions core/extension/gdextension.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ void GDExtension::_register_extension_class(GDExtensionClassLibraryPtr p_library
p_extension_funcs->set_func, // GDExtensionClassSet set_func;
p_extension_funcs->get_func, // GDExtensionClassGet get_func;
p_extension_funcs->get_property_list_func, // GDExtensionClassGetPropertyList get_property_list_func;
p_extension_funcs->free_property_list_func, // GDExtensionClassFreePropertyList free_property_list_func;
nullptr, // GDExtensionClassFreePropertyList2 free_property_list_func;
p_extension_funcs->property_can_revert_func, // GDExtensionClassPropertyCanRevert property_can_revert_func;
p_extension_funcs->property_get_revert_func, // GDExtensionClassPropertyGetRevert property_get_revert_func;
nullptr, // GDExtensionClassValidateProperty validate_property_func;
Expand All @@ -406,7 +406,8 @@ void GDExtension::_register_extension_class(GDExtensionClassLibraryPtr p_library
};

const ClassCreationDeprecatedInfo legacy = {
p_extension_funcs->notification_func,
p_extension_funcs->notification_func, // GDExtensionClassNotification notification_func;
p_extension_funcs->free_property_list_func, // GDExtensionClassFreePropertyList free_property_list_func;
};
_register_extension_class_internal(p_library, p_class_name, p_parent_class_name, &class_info3, &legacy);
}
Expand All @@ -420,7 +421,7 @@ void GDExtension::_register_extension_class2(GDExtensionClassLibraryPtr p_librar
p_extension_funcs->set_func, // GDExtensionClassSet set_func;
p_extension_funcs->get_func, // GDExtensionClassGet get_func;
p_extension_funcs->get_property_list_func, // GDExtensionClassGetPropertyList get_property_list_func;
p_extension_funcs->free_property_list_func, // GDExtensionClassFreePropertyList free_property_list_func;
nullptr, // GDExtensionClassFreePropertyList2 free_property_list_func;
p_extension_funcs->property_can_revert_func, // GDExtensionClassPropertyCanRevert property_can_revert_func;
p_extension_funcs->property_get_revert_func, // GDExtensionClassPropertyGetRevert property_get_revert_func;
p_extension_funcs->validate_property_func, // GDExtensionClassValidateProperty validate_property_func;
Expand All @@ -438,7 +439,11 @@ void GDExtension::_register_extension_class2(GDExtensionClassLibraryPtr p_librar
p_extension_funcs->class_userdata, // void *class_userdata;
};

_register_extension_class_internal(p_library, p_class_name, p_parent_class_name, &class_info3);
const ClassCreationDeprecatedInfo legacy = {
nullptr, // GDExtensionClassNotification notification_func;
p_extension_funcs->free_property_list_func, // GDExtensionClassFreePropertyList free_property_list_func;
};
_register_extension_class_internal(p_library, p_class_name, p_parent_class_name, &class_info3, &legacy);
}
#endif // DISABLE_DEPRECATED

Expand Down Expand Up @@ -514,13 +519,14 @@ void GDExtension::_register_extension_class_internal(GDExtensionClassLibraryPtr
extension->gdextension.set = p_extension_funcs->set_func;
extension->gdextension.get = p_extension_funcs->get_func;
extension->gdextension.get_property_list = p_extension_funcs->get_property_list_func;
extension->gdextension.free_property_list = p_extension_funcs->free_property_list_func;
extension->gdextension.free_property_list2 = p_extension_funcs->free_property_list_func;
extension->gdextension.property_can_revert = p_extension_funcs->property_can_revert_func;
extension->gdextension.property_get_revert = p_extension_funcs->property_get_revert_func;
extension->gdextension.validate_property = p_extension_funcs->validate_property_func;
#ifndef DISABLE_DEPRECATED
if (p_deprecated_funcs) {
extension->gdextension.notification = p_deprecated_funcs->notification_func;
extension->gdextension.free_property_list = p_deprecated_funcs->free_property_list_func;
}
#endif // DISABLE_DEPRECATED
extension->gdextension.notification2 = p_extension_funcs->notification_func;
Expand Down
1 change: 1 addition & 0 deletions core/extension/gdextension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class GDExtension : public Resource {
struct ClassCreationDeprecatedInfo {
#ifndef DISABLE_DEPRECATED
GDExtensionClassNotification notification_func = nullptr;
GDExtensionClassFreePropertyList free_property_list_func = nullptr;
#endif // DISABLE_DEPRECATED
};

Expand Down
3 changes: 2 additions & 1 deletion core/extension/gdextension_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ typedef struct {

typedef const GDExtensionPropertyInfo *(*GDExtensionClassGetPropertyList)(GDExtensionClassInstancePtr p_instance, uint32_t *r_count);
typedef void (*GDExtensionClassFreePropertyList)(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list);
typedef void (*GDExtensionClassFreePropertyList2)(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list, uint32_t p_count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Raul pointed this out on the godot-cpp PR - the const here is actually incorrect (because we're freeing the memory this points at), and we're working around it in godot-cpp with a const_cast. This would also be a good time to fix that as well!

Suggested change
typedef void (*GDExtensionClassFreePropertyList2)(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list, uint32_t p_count);
typedef void (*GDExtensionClassFreePropertyList2)(GDExtensionClassInstancePtr p_instance, GDExtensionPropertyInfo *p_list, uint32_t p_count);

Copy link
Contributor

@dsnopek dsnopek Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Or, hrm... Thinking about this a little bit more, I'm not sure const is incorrect. If we remove the const, then Godot will have to do the const_cast in order to call this function!

get_property_list returns const GDExtensionPropertyInfo * because we don't want Godot to modify this data, it's "owned" by the GDExtension. But since it will have a const GDExtensionPropertyInfo *, Godot can't call free_property_list with a GDExtensionPropertyInfo * without casting it. But, arguably, it'd be better for the GDExtension to do the cast because it's the one that "owns" this data, not Godot.

So, on second thought, maybe const is correct, and we need to update the comment in godot-cpp about it being incorrect?

Copy link

Choose a reason for hiding this comment

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

isn't it then get_property_list that's wrong? i think it returns a const pointer, but that pointer is later gonna be freed so it shouldn't be a const pointer?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's getting returned to Godot, though, and it's not Godot that's going to free it. It's going to pass it back to the GDExtension to do the freeing. From the perspective of Godot, I feel like a const pointer is correct.

Copy link

Choose a reason for hiding this comment

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

from the perspective of godot, isn't this effectively the same as a normal alloc + free (except that you're given initialized memory here)? like sure the gdextension is who actually performs the logic to do the freeing, but the same is true for calling the free function for a memory allocator. the memory allocator is actually the one freeing the memory but we're still responsible for triggering that logic by calling free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my understanding, "able to free" doesn't imply "must not be const".
This works fine in C++:

const int* p = new int(30);
delete p;

Now, C free() does take a non-const pointer, but that's likely due to its age.
There's also some discussion on StackOverflow.

typedef GDExtensionBool (*GDExtensionClassPropertyCanRevert)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name);
typedef GDExtensionBool (*GDExtensionClassPropertyGetRevert)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret);
typedef GDExtensionBool (*GDExtensionClassValidateProperty)(GDExtensionClassInstancePtr p_instance, GDExtensionPropertyInfo *p_property);
Expand Down Expand Up @@ -333,7 +334,7 @@ typedef struct {
GDExtensionClassSet set_func;
GDExtensionClassGet get_func;
GDExtensionClassGetPropertyList get_property_list_func;
GDExtensionClassFreePropertyList free_property_list_func;
GDExtensionClassFreePropertyList2 free_property_list_func;
GDExtensionClassPropertyCanRevert property_can_revert_func;
GDExtensionClassPropertyGetRevert property_get_revert_func;
GDExtensionClassValidateProperty validate_property_func;
Expand Down
5 changes: 3 additions & 2 deletions core/object/class_db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class PlaceholderExtensionInstance {
return nullptr;
}

static void placeholder_instance_free_property_list(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list) {
static void placeholder_instance_free_property_list(GDExtensionClassInstancePtr p_instance, const GDExtensionPropertyInfo *p_list, uint32_t p_count) {
}

static GDExtensionBool placeholder_instance_property_can_revert(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name) {
Expand Down Expand Up @@ -600,12 +600,13 @@ ObjectGDExtension *ClassDB::get_placeholder_extension(const StringName &p_class)
placeholder_extension->set = &PlaceholderExtensionInstance::placeholder_instance_set;
placeholder_extension->get = &PlaceholderExtensionInstance::placeholder_instance_get;
placeholder_extension->get_property_list = &PlaceholderExtensionInstance::placeholder_instance_get_property_list;
placeholder_extension->free_property_list = &PlaceholderExtensionInstance::placeholder_instance_free_property_list;
placeholder_extension->free_property_list2 = &PlaceholderExtensionInstance::placeholder_instance_free_property_list;
placeholder_extension->property_can_revert = &PlaceholderExtensionInstance::placeholder_instance_property_can_revert;
placeholder_extension->property_get_revert = &PlaceholderExtensionInstance::placeholder_instance_property_get_revert;
placeholder_extension->validate_property = &PlaceholderExtensionInstance::placeholder_instance_validate_property;
#ifndef DISABLE_DEPRECATED
placeholder_extension->notification = nullptr;
placeholder_extension->free_property_list = nullptr;
#endif // DISABLE_DEPRECATED
placeholder_extension->notification2 = &PlaceholderExtensionInstance::placeholder_instance_notification;
placeholder_extension->to_string = &PlaceholderExtensionInstance::placeholder_instance_to_string;
Expand Down
7 changes: 6 additions & 1 deletion core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,9 +503,14 @@ void Object::get_property_list(List<PropertyInfo> *p_list, bool p_reversed) cons
for (uint32_t i = 0; i < pcount; i++) {
p_list->push_back(PropertyInfo(pinfo[i]));
}
if (current_extension->free_property_list) {
if (current_extension->free_property_list2) {
current_extension->free_property_list2(_extension_instance, pinfo, pcount);
}
#ifndef DISABLE_DEPRECATED
else if (current_extension->free_property_list) {
current_extension->free_property_list(_extension_instance, pinfo);
}
#endif // DISABLE_DEPRECATED
#ifdef TOOLS_ENABLED
}
#endif
Expand Down
3 changes: 2 additions & 1 deletion core/object/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,12 +324,13 @@ struct ObjectGDExtension {
GDExtensionClassSet set;
GDExtensionClassGet get;
GDExtensionClassGetPropertyList get_property_list;
GDExtensionClassFreePropertyList free_property_list;
GDExtensionClassFreePropertyList2 free_property_list2;
GDExtensionClassPropertyCanRevert property_can_revert;
GDExtensionClassPropertyGetRevert property_get_revert;
GDExtensionClassValidateProperty validate_property;
#ifndef DISABLE_DEPRECATED
GDExtensionClassNotification notification;
GDExtensionClassFreePropertyList free_property_list;
#endif // DISABLE_DEPRECATED
GDExtensionClassNotification2 notification2;
GDExtensionClassToString to_string;
Expand Down