GDExtension: Provide free_property_list_func with length of array

[Bromeon]

Closes godotengine/godot-proposals#9462.

That issue contains a detailed rationale, but the TLDR is that the length information helps the binding side to know the length of the allocation. It is the equivalent of #89055 but for extension class methods, not script instance methods.

Would be nice to include in 4.3, as we'd have to introduce yet another struct version just for this otherwise 🙂

[dsnopek]

Thanks!

Overall, the code looks great to me! (Although the nit that Raul points out should be fixed.)

I tested this with godot-cpp master (plus PR godotengine/godot-cpp#1450) and with the Godot 4.2 version of godot-cpp, in order to test that the legacy support is working. Everything seemed to work fine!

I agree that it would be nice to get this in Godot 4.3, both so we don't have to make a new struct and because it matches the script instance changes also made for Godot 4.3. I think this change is simple and small enough that it should be safe to do.

[dsnopek]

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);

[dsnopek]

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?

[lilizoey]

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?

[dsnopek]

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.

[lilizoey]

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.

[Bromeon]

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.

[akien-mga]

Thanks!