GDExtension: PtrToArg::convert() uses const-reference where possible

[Bromeon]

Fixes #80074.

In the cases where reinterpret_cast is immediately dereferenced, PtrToArg::convert() returns a const-reference instead of a value. This allows to reuse the object located at the address of the passed const void* pointer, instead of triggering the copy constructor.

We cannot return const-references where temporary values are constructed. Most manual PtrToArg specializations fall under this category and are thus untouched.

In some cases, we now return a const-reference to scalar types like int, bool etc. Since the function is annotated as _FORCE_INLINE_, I expect any such indirections to be optimized away though.

I did some tests with the Rust binding (gdext) already, but I'd appreciate some input on this!

[Bromeon]

This is purely a whitespace change (should be more readable and still OK for clang-format) 🙂

[Bromeon]

CI job Editor with doubles and GCC sanitizers failed with:

ranlib: /home/runner/work/godot/godot/godot-cpp/bin/libgodot-cpp.linux.template_debug.dev.x86_64.a: No space left on device

I encountered the same issue in my own nightly builds, also gcc memcheck running out of disk space... Was there a regression? Or GitHub decreased available disk space?

[dalexeev]

@Bromeon See:

• CI keeps outdated caches until it runs out of space, so we should delete them manually #79919
• #buildsystem channel on Godot Contributors Chat.

[dsnopek]

Good catch!

This makes sense to me. Even outside the context of GDExtension, we do ptrcalls with code like:

template <class T, class... P, size_t... Is>
void call_with_ptr_args_helper(T *p_instance, void (T::*p_method)(P...), const void **p_args, IndexSequence<Is...>) {
	(p_instance->*p_method)(PtrToArg<P>::convert(p_args[Is])...);
}

In the case that one (or more) of the method's arguments takes a const &, this would also avoid a copy.

I ran godot-cpp's automated tests and they passed. So, this seems OK from the perspective of GDExtension. It could probably use to be reviewed by the core team as well.

[Bromeon]

Rebased onto master.

[dsnopek]

Discussed at the GDExtension meeting, and we think this makes sense. This is also on the agenda to discuss at the core team meeting, where we hope to double check with them that this won't cause any other problems.

[jsjtxietian]

Great Catch!
For anyone who is intersted, I made a small demo in compiler explorer, even with -O3 the copy can not be elimated.
https://godbolt.org/z/r4KfGdh1h

[dsnopek]

The core team meeting I was hoping to discuss this at didn't end up really happening. It'd still be nice to have a review from someone on the core team, however, I personally really think this should be OK. This is adding const & in a template and so I think any issues would be detected at compile time, and this is compiling just fine on all platforms in the CI.

[Bromeon]

For anyone who is intersted, I made a small demo in compiler explorer, even with -O3 the copy can not be elimated.

The compiler has to be conservative, because the pointer passed to convert() is opaque: it cannot know what happens to it once we've made a copy. This copy could as well be deliberate because we're intending to change the original object. The copy itself could have side effects (which might be disproved by inspecting the copy constructor implementation; not sure how deep compilers go here).

It is our "insider knowledge" that a GDExtension binding keeps that pointer unchanged, as it's in a function call.

[akien-mga]

Couldn't get another review from the core team, but it looks overall sensible to me and I trust that if there's any issue with this change, we'll find out fast.

[akien-mga]

Thanks!