Skip to content
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

[3.4.2] register_method does not accept arguments passed by reference #697

Open
Lecrapouille opened this issue Feb 14, 2022 · 4 comments
Open

Comments

@Lecrapouille
Copy link

Lecrapouille commented Feb 14, 2022

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 Lecrapouille changed the title register_method does not accept arguments passed by reference [3.4.2] register_method does not accept arguments passed by reference Feb 14, 2022
@Lecrapouille
Copy link
Author

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

@Lecrapouille
Copy link
Author

Lecrapouille commented Feb 15, 2022

@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 ?

@Calinou
Copy link
Member

Calinou commented Feb 15, 2022

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.

@Lecrapouille
Copy link
Author

That makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants