godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

[3.4.2] register_method does not accept arguments passed by reference

Open Lecrapouille opened this issue 3 years ago • 4 comments

Let suppose a member method like this:

void Foo::bar(godot::String const name);

If we want to make it uses for GDNative script, we have to export it:

godot::register_method("bar", &Foo::bar);

This makes pass the string by copy and this works fine ! But now let pass name by reference:

void Foo::bar(godot::String const& name);

But this reference will produce a warning:

...

/home/qq/godot/3.4.2-stable/cpp/include/core/Godot.hpp:163:10: warning: returning reference to temporary [-Wreturn-local-addr]
  163 |   return a;
      |          ^

And possibly gives a crash. So How to deal: passing C++ reference with register_method ?

PS: register_method and C++ references worked well within Godot modules (C++ code inside the Godot editor code source inside the modules/ folder). I had no warnings.

Lecrapouille avatar Feb 14 '22 23:02 Lecrapouille

@Calinou I rephrase my sentence because it was not very understandable.

Lecrapouille avatar Feb 14 '22 23:02 Lecrapouille

@Calinou Maybe a patch could be in godot-cpp/include/core/Godot.hpp (godot 3.4.2) where the current code is:

template <class T>
struct _ArgCast {
	static T _arg_cast(Variant a) {
		return a;                                  <---- this is my line 163
	}
};

template <class T>
struct _ArgCast<T *> {
	static T *_arg_cast(Variant a) {
		return (T *)T::___get_from_variant(a);
	}
};

template <>
struct _ArgCast<Variant> {
	static Variant _arg_cast(Variant a) {
		return a;
	}
};

To add this:

template <class T>
struct _ArgCast<T &> {
	static T _arg_cast(Variant a) {
		return a;
	}
};

I have no more warning, but I very unsure if this is the good solution since in my opinion this will make an extra copy that we do not want.

PS: why not these methods are not constexpr this could help avoiding making copies ?

Lecrapouille avatar Feb 15 '22 01:02 Lecrapouille

PS: why not these methods are not constexpr this could help avoiding making copies ?

constexpr is a C++11 feature (or C++14/C++17 depending on context), and GDNative in Godot 3.x was largely designed around C++03.

In contrast, GDExtension in 4.0 can mandate C++17 support since Godot 4.0 itself requires a C++ compiler to be built.

Calinou avatar Feb 15 '22 18:02 Calinou

That makes sense

Lecrapouille avatar Feb 15 '22 19:02 Lecrapouille