GDExtension C Example - Vector2 misleadingly not cleaned up

[charjr]

Problem

Within the section on registering a signal a helper function is defined which states afterwards:

At the end we need to destruct the Variants we created. While technically the
Vector2 one does not require destructing, it is clearer to cleanup everything.

There are two issues:

1. This message is misleading, as it makes me think it has been cleaned up.
2. Nothing explains why Vector2 doesn't need to be destroyed but other things do.

godot-docs/tutorials/scripting/gdextension/gdextension_c_example.rst

Lines 1998 to 2030 in e11d59a

	void call_2_args_stringname_vector2_no_ret_variant(GDExtensionMethodBindPtr p_method_bind, GDExtensionObjectPtr p_instance, const GDExtensionTypePtr p_arg1, const GDExtensionTypePtr p_arg2)
	{
	// Set up the arguments for the call.
	Variant arg1;
	constructors.variant_from_string_name_constructor(&arg1, p_arg1);
	Variant arg2;
	constructors.variant_from_vector2_constructor(&arg2, p_arg2);
	GDExtensionConstVariantPtr args[] = {&arg1, &arg2};
	// Add dummy return value storage.
	Variant ret;
	// Call the function.
	api.object_method_bind_call(p_method_bind, p_instance, args, 2, &ret, NULL);
	// Destroy the arguments that need it.
	destructors.variant_destroy(&arg1);
	destructors.variant_destroy(&ret);
	}
	This helper function has some boilerplate code but is quite straightforward. It sets up the
	two arguments inside stack allocated Variants, then creates an array with
	pointers to those. It also sets up another Variant to keep the return value,
	which we don't need to construct since the call expects it to be uninitialized.
	Then it actually calls the MethodBind using the instance we provided and the
	arguments. The ``NULL`` at the end would be a pointer to a
	``GDExtensionCallError`` struct. This can be used to treat potential errors when
	calling the functions (such as wrong arguments). For the sake of simplicity
	we're not gonna handle that here.
	At the end we need to destruct the Variants we created. While technically the
	Vector2 one does not require destructing, it is clearer to cleanup everything.

Solution

Make it clear that it hasn't been cleaned up but could be (presumably at the expense of more writing more boilerplate).

Alternatively, actually destroy it in the code (if it doesn't expand the docs much).

Ideally, it would be nice to know why Vector2 doesn't need to be destroyed, I noticed in an earlier step it wasn't destroyed either and this was a point of confusion for me.

[Calinou]

Nothing explains why Vector2 doesn't need to be destroyed but other things do.

To my knowledge, only Object-derived types need to be destroyed (Vector2 is a Variant type).

[charjr]

To my knowledge, only Object-derived types need to be destroyed (Vector2 is a Variant type).

I trust your expertise and what you say conflicts with the documentation:

For now it only has a pointer to a function that creates a StringName from a C string (in Latin-1 encoding) and another to destruct a StringName, which we'll need to use to avoid leaking memory,

GDExtension C Example (Just below the Wrapper API heading)

As someone who still relies on reading the docs:

• I'm told StringName (a Variant type) needs to be destroyed or it will leak memory.
• I'm told Vector2 (a Variant type) does not need to be destroyed.

I think there needs to be clarification on what does/doesn't need destroying when writing in C (if that clarification already exists, then reference it with a link)

[AThousandShips]

They are different types, with different memory behavior, one contains a pointer to memory, one doesn't

[AThousandShips]

Here is a list of the types and if they need explicit deinitialization, if this has a false the destructor does nothing
https://github.com/godotengine/godot/blob/c0c1c685621971c75c0f0f4aab85e8293c67a993/core/variant/variant.h#L292-L336

I think the documentation is clear and makes sense, it says you don't need to destruct them, and that's important information, but does it to give an example of good practice, especially when you're not certain

[charjr]

Here is a list of the types and if they need explicit deinitialization, if this has a false the destructor does nothing https://github.com/godotengine/godot/blob/c0c1c685621971c75c0f0f4aab85e8293c67a993/core/variant/variant.h#L292-L336

That is incredible, thank you!

I think the documentation is clear and makes sense,

I agree, it's very well written. Thank you to every contributer involved. 🙏

it says you don't need to destruct them, and that's important information,

Which is very helpful, and thank you for showing me where to look in the internals.

but does it to give an example of good practice, especially when you're not certain.

It states what that good practice is, but the code does not show that good practice:

         // Destroy the arguments that need it. 
         destructors.variant_destroy(&arg1); 
         destructors.variant_destroy(&ret); 
...

While technically the 
 Vector2 one does not require destructing, it is clearer to cleanup everything. 

Ideally, I would just want to see something like this.

         // Destroy the arguments. 
         destructors.variant_destroy(&arg1); // Required.
         destructors.variant_destroy(&arg2); // Not required.
         destructors.variant_destroy(&ret); // Required.

I can put in a PR for this later if this seems sensible.

[AThousandShips]

Would be good to assess if the return value does need destroying, it shouldn't be written to if the function doesn't return, it's just a dummy

[charjr]

How would this be tested/assessed? If I'm given the direction I'd be happy to do the checking as it would be a valuable learning experience for me in the process. 🤓