Refactored binding system for core types #42780

reduz · 2020-10-13T19:02:45Z

Moved to a system using variadic templates, shared with CallableBind.

New code is cleaner, faster and allows for much better optimization of core
type functions from GDScript and GDNative. Also adds support for prevalidated and pointer calls
to internal methods.

Added Variant::InternalMethod function for direct call access.

Note: while it should build ok this PR breaks release target (I think its broken already..), a subsequent PR refactoring method binds should fix it.

core/callable_method_pointer.h

core/variant_call.cpp

core/variant_internal.h

core/ustring.cpp

vnen

This will be really nice for GDScript

akien-mga · 2020-10-14T06:53:05Z

core/variant_call.cpp

+_FORCE_INLINE_ void sarray_add_str(Vector<String> &arr) {
+}


Is this actually used? Or is it something necessary to make the template magic work?

akien-mga · 2020-10-14T07:22:51Z

Getting more warnings locally with GCC 10.2.0 which the CI somehow seemed to miss:

-Werror=type-limits

core/variant_call.cpp: In instantiation of 'void _VariantCall::InternalMethodRC<T, R, P>::call(Variant*, const Variant**, int, Variant&, Callable::CallError&) [with T = Vector<Color>; R = Vector<unsigned char>; P = {}]':
core/variant_call.cpp:288:16:   required from here
core/variant_call.cpp:301:27: error: comparison of unsigned expression in '< 0' is always false [-Werror=type-limits]
  301 |      for (size_t i = 0; i < sizeof...(P); i++) {
      |                         ~~^~~~~~~~~~~~~~
core/variant_call.cpp: In instantiation of 'void _VariantCall::InternalMethod<T, P>::call(Variant*, const Variant**, int, Variant&, Callable::CallError&) [with T = Vector<Color>; P = {}]':
core/variant_call.cpp:98:16:   required from here
core/variant_call.cpp:111:27: error: comparison of unsigned expression in '< 0' is always false [-Werror=type-limits]
  111 |      for (size_t i = 0; i < sizeof...(P); i++) {
      |                         ~~^~~~~~~~~~~~~~
core/variant_call.cpp: In instantiation of 'void _VariantCall::InternalMethodRC<T, R, P>::call(Variant*, const Variant**, int, Variant&, Callable::CallError&) [with T = Vector<Color>; R = bool; P = {}]':
core/variant_call.cpp:288:16:   required from here
core/variant_call.cpp:301:27: error: comparison of unsigned expression in '< 0' is always false [-Werror=type-limits]
  301 |      for (size_t i = 0; i < sizeof...(P); i++) {
      |                         ~~^~~~~~~~~~~~~~

<repeated dozens of times>

-Werror=unused-but-set-parameter

In file included from ./core/class_db.h:44,
                 from ./core/reference.h:34,
                 from ./core/crypto/crypto_core.h:34,
                 from core/variant_call.cpp:35:
./core/callable_method_pointer.h: In instantiation of 'Variant::Type call_get_argument_type(int) [with P = {}]':
core/variant_call.cpp:261:39:   required from 'Variant::Type _VariantCall::InternalMethodRC<T, R, P>::get_argument_type(int) const [with T = Vector<Color>; R = Vector<unsigned char>; P = {}]'
core/variant_call.cpp:260:25:   required from here
./core/callable_method_pointer.h:259:42: error: parameter 'p_arg' set but not used [-Werror=unused-but-set-parameter]
  259 | Variant::Type call_get_argument_type(int p_arg) {
      |                                      ~~~~^~~~~
./core/callable_method_pointer.h: In instantiation of 'void call_with_variant_args_retc_static_helper(T*, R (*)(T*, P ...), const Variant**, Variant&, Callable::CallError&, IndexSequence<Is ...>) [with T = Vector<unsigned char>; R = String; P = {}; long unsigned int ...Is = {}]':
core/variant_call.cpp:410:45:   required from 'void _VariantCall::InternalMethodRS<T, R, P>::call(Variant*, const Variant**, int, Variant&, Callable::CallError&) [with T = Vector<unsigned char>; R = String; P = {}]'
core/variant_call.cpp:382:16:   required from here
./core/callable_method_pointer.h:535:105: error: parameter 'p_args' set but not used [-Werror=unused-but-set-parameter]
  535 | void call_with_variant_args_retc_static_helper(T *p_instance, R (*p_method)(T *, P...), const Variant **p_args, Variant &r_ret, Callable::CallError &r_error, IndexSequence<Is...>) {
      |                                                                                         ~~~~~~~~~~~~~~~~^~~~~~

I'll see if I can fix them myself.

akien-mga · 2020-10-14T08:41:09Z

I force pushed an amend that should fix the remaining GCC warnings.

Diff with the previous commit:

diff --git a/core/callable_method_pointer.h b/core/callable_method_pointer.h
index a49e8b61ec..a2275452b4 100644
--- a/core/callable_method_pointer.h
+++ b/core/callable_method_pointer.h
@@ -116,7 +116,7 @@ struct VariantCasterAndValidate<const T &> {
 
 #endif // DEBUG_METHODS_ENABLED
 
-// GCC 8 raises "parameter 'p_args' set but not used" here, probably using a
+// GCC raises "parameter 'p_args' set but not used" here, probably using a
 // template version that does not have arguments and thus sees it unused, but
 // obviously the template can be used for functions with and without them, and
 // the optimizer will get rid of it anyway.
@@ -158,7 +158,8 @@ void call_with_ptr_args_static_retc_helper(T *p_instance, R (*p_method)(T *, P..
 	PtrToArg<R>::encode(p_method(p_instance, PtrToArg<P>::convert(p_args[Is])...), r_ret);
 }
 
-#endif
+#endif // PTRCALL_ENABLED
+
 template <class T, class... P, size_t... Is>
 void call_with_validated_variant_args_helper(T *p_instance, void (T::*p_method)(P...), const Variant **p_args, IndexSequence<Is...>) {
 	(p_instance->*p_method)((VariantInternalAccessor<typename GetSimpleTypeT<P>::type_t>::get(p_args[Is]))...);
@@ -179,10 +180,6 @@ void call_with_validated_variant_args_static_retc_helper(T *p_instance, R (*p_me
 	VariantInternalAccessor<typename GetSimpleTypeT<R>::type_t>::set(r_ret, p_method(p_instance, (VariantInternalAccessor<typename GetSimpleTypeT<P>::type_t>::get(p_args[Is]))...));
 }
 
-#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 template <class T, class... P>
 void call_with_variant_args(T *p_instance, void (T::*p_method)(P...), const Variant **p_args, int p_argcount, Callable::CallError &r_error) {
 #ifdef DEBUG_METHODS_ENABLED
@@ -223,7 +220,7 @@ void call_with_ptr_args_static_retc(T *p_instance, R (*p_method)(T *, P...), con
 	call_with_ptr_args_static_retc_helper<T, R, P...>(p_instance, p_method, p_args, r_ret, BuildIndexSequence<sizeof...(P)>{});
 }
 
-#endif
+#endif // PTRCALL_ENABLED
 
 template <class T, class... P>
 void call_with_validated_variant_args(Variant *base, void (T::*p_method)(P...), const Variant **p_args) {
@@ -255,18 +252,28 @@ void call_get_argument_type_helper(int p_arg, int &index, Variant::Type &type) {
 	index++;
 }
 
+// GCC's warnings checker really doesn't like variadic voodoo.
+// It sees `index` unused below in some branches, so it raises a warning.
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
+#endif
+
 template <class... P>
 Variant::Type call_get_argument_type(int p_arg) {
 	Variant::Type type = Variant::NIL;
 	int index = 0;
-	// I think rocket science is simpler than modern C++
+	// I think rocket science is simpler than modern C++.
 	using expand_type = int[];
 	expand_type a{ 0, (call_get_argument_type_helper<P>(p_arg, index, type), 0)... };
-	(void)a; //supress warnings
-	(void)index; //gcc is broken
+	(void)a; // Suppress (valid, but unavoidable) -Wunused-variable warning.
 	return type;
 }
 
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
 #else
 
 template <class... P>
@@ -274,7 +281,7 @@ Variant::Type call_get_argument_type(int p_arg) {
 	return Variant::NIL;
 }
 
-#endif
+#endif // DEBUG_METHODS_ENABLED
 
 template <class T, class... P>
 class CallableCustomMethodPointer : public CallableCustomMethodPointerBase {
@@ -331,15 +338,6 @@ Callable create_custom_callable_function_pointer(T *p_instance,
 
 // VERSION WITH RETURN
 
-// GCC 8 raises "parameter 'p_args' set but not used" here, probably using a
-// template version that does not have arguments and thus sees it unused, but
-// obviously the template can be used for functions with and without them, and
-// the optimizer will get rid of it anyway.
-#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
-#endif
-
 template <class T, class R, class... P, size_t... Is>
 void call_with_variant_args_ret_helper(T *p_instance, R (T::*p_method)(P...), const Variant **p_args, Variant &r_ret, Callable::CallError &r_error, IndexSequence<Is...>) {
 	r_error.error = Callable::CallError::CALL_OK;
@@ -351,10 +349,6 @@ void call_with_variant_args_ret_helper(T *p_instance, R (T::*p_method)(P...), co
 #endif
 }
 
-#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 template <class T, class R, class... P>
 void call_with_variant_args_ret(T *p_instance, R (T::*p_method)(P...), const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error) {
 #ifdef DEBUG_METHODS_ENABLED
@@ -429,15 +423,6 @@ Callable create_custom_callable_function_pointer(T *p_instance,
 
 // CONST VERSION WITH RETURN
 
-// GCC 8 raises "parameter 'p_args' set but not used" here, probably using a
-// template version that does not have arguments and thus sees it unused, but
-// obviously the template can be used for functions with and without them, and
-// the optimizer will get rid of it anyway.
-#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
-#endif
-
 template <class T, class R, class... P, size_t... Is>
 void call_with_variant_args_retc_helper(T *p_instance, R (T::*p_method)(P...) const, const Variant **p_args, Variant &r_ret, Callable::CallError &r_error, IndexSequence<Is...>) {
 	r_error.error = Callable::CallError::CALL_OK;
@@ -449,10 +434,6 @@ void call_with_variant_args_retc_helper(T *p_instance, R (T::*p_method)(P...) co
 #endif
 }
 
-#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 template <class T, class R, class... P>
 void call_with_variant_args_retc(T *p_instance, R (T::*p_method)(P...) const, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error) {
 #ifdef DEBUG_METHODS_ENABLED
@@ -542,4 +523,8 @@ void call_with_variant_args_retc_static_helper(T *p_instance, R (*p_method)(T *,
 #endif
 }
 
+#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
 #endif // CALLABLE_METHOD_POINTER_H
diff --git a/core/math/plane.h b/core/math/plane.h
index 6017cdb5c1..1386b0a2cb 100644
--- a/core/math/plane.h
+++ b/core/math/plane.h
@@ -61,7 +61,7 @@ public:
 	bool intersects_ray(const Vector3 &p_from, const Vector3 &p_dir, Vector3 *p_intersection) const;
 	bool intersects_segment(const Vector3 &p_begin, const Vector3 &p_end, Vector3 *p_intersection) const;
 
-	//for Variant binding
+	// For Variant bindings.
 	Variant intersect_3_bind(const Plane &p_plane1, const Plane &p_plane2) const;
 	Variant intersects_ray_bind(const Vector3 &p_from, const Vector3 &p_dir) const;
 	Variant intersects_segment_bind(const Vector3 &p_begin, const Vector3 &p_end) const;
diff --git a/core/ustring.cpp b/core/ustring.cpp
index 98fd78a06f..27dab8db6e 100644
--- a/core/ustring.cpp
+++ b/core/ustring.cpp
@@ -4763,6 +4763,7 @@ Vector<uint8_t> String::to_utf8_buffer() const {
 
 	return retval;
 }
+
 Vector<uint8_t> String::to_utf16_buffer() const {
 	const String *s = this;
 	if (s->empty()) {
diff --git a/core/variant_call.cpp b/core/variant_call.cpp
index 5bc5d543df..8246ccf2b0 100644
--- a/core/variant_call.cpp
+++ b/core/variant_call.cpp
@@ -108,6 +108,13 @@ struct _VariantCall {
 				size_t missing = sizeof...(P) - (size_t)p_argcount;
 				if (missing <= (size_t)default_values.size()) {
 					args = (const Variant **)alloca(sizeof...(P) * sizeof(const Variant *));
+					// GCC fails to see that `sizeof...(P)` cannot be 0 here given the previous
+					// conditions, so it raises a warning on the potential use of `i < 0` as the
+					// execution condition.
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wtype-limits"
+#endif
 					for (size_t i = 0; i < sizeof...(P); i++) {
 						if (i < (size_t)p_argcount) {
 							args[i] = p_args[i];
@@ -115,6 +122,9 @@ struct _VariantCall {
 							args[i] = &default_values[i - p_argcount + (default_values.size() - missing)];
 						}
 					}
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
 				} else {
 #ifdef DEBUG_ENABLED
 					r_error.error = Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS;
@@ -298,6 +308,13 @@ struct _VariantCall {
 				size_t missing = sizeof...(P) - (size_t)p_argcount;
 				if (missing <= (size_t)default_values.size()) {
 					args = (const Variant **)alloca(sizeof...(P) * sizeof(const Variant *));
+					// GCC fails to see that `sizeof...(P)` cannot be 0 here given the previous
+					// conditions, so it raises a warning on the potential use of `i < 0` as the
+					// execution condition.
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wtype-limits"
+#endif
 					for (size_t i = 0; i < sizeof...(P); i++) {
 						if (i < (size_t)p_argcount) {
 							args[i] = p_args[i];
@@ -305,6 +322,9 @@ struct _VariantCall {
 							args[i] = &default_values[i - p_argcount + (default_values.size() - missing)];
 						}
 					}
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
 				} else {
 #ifdef DEBUG_ENABLED
 					r_error.error = Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS;
@@ -392,6 +412,13 @@ struct _VariantCall {
 				size_t missing = sizeof...(P) - (size_t)p_argcount;
 				if (missing <= (size_t)default_values.size()) {
 					args = (const Variant **)alloca(sizeof...(P) * sizeof(const Variant *));
+					// GCC fails to see that `sizeof...(P)` cannot be 0 here given the previous
+					// conditions, so it raises a warning on the potential use of `i < 0` as the
+					// execution condition.
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wtype-limits"
+#endif
 					for (size_t i = 0; i < sizeof...(P); i++) {
 						if (i < (size_t)p_argcount) {
 							args[i] = p_args[i];
@@ -399,6 +426,9 @@ struct _VariantCall {
 							args[i] = &default_values[i - p_argcount + (default_values.size() - missing)];
 						}
 					}
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
 				} else {
 #ifdef DEBUG_ENABLED
 					r_error.error = Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS;

akien-mga · 2020-10-14T08:44:34Z

core/callable_method_pointer.h

+	// I think rocket science is simpler than modern C++
+	using expand_type = int[];
+	expand_type a{ 0, (call_get_argument_type_helper<P>(p_arg, index, type), 0)... };
+	(void)a; // Suppress (valid, but unavoidable) -Wunused-variable warning.


For the reference, I think the warning is valid here, as we do not use a. But I assume this is an unavoidable hack to get things to work as intended (hence the rocket science comment).

We could silence it with a pragma, but then it needs to be handled for both GCC and Clang (and maybe MSVC?), so this hack is likely better.

akien-mga · 2020-10-14T08:49:55Z

core/variant_call.cpp

+					// GCC fails to see that `sizeof...(P)` cannot be 0 here given the previous
+					// conditions, so it raises a warning on the potential use of `i < 0` as the
+					// execution condition.
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wtype-limits"
+#endif
+					for (size_t i = 0; i < sizeof...(P); i++) {


This might be worth a bug report to upstream GCC IMO, if anyone is motivated to write a minimal reproduction project (basically some code that uses variadic templates with a parameter pack that can be of size 0, but which has a check for its size before attempting to use it in a comparison with a size_t).

akien-mga · 2020-10-14T09:14:26Z

Here's the output of running doctool after this PR: https://hastebin.com/ewoxetinir.diff

Lots of methods removed from core types, I guess the doc system is not compatible yet with this new binding system?

It should be fixed in priority to avoid losing valuable descriptions when running doctool.

Moved to a system using variadic templates, shared with CallableBind. New code is cleaner, faster and allows for much better optimization of core type functions from GDScript and GDNative. Added Variant::InternalMethod function for direct call access.

akien-mga · 2020-10-14T13:29:09Z

Force pushed to fix a new warning introduced in recent changes.

There are bugs in the doctool output: https://paste.centos.org/view/2a4caa04
But @reduz will fix those in the follow up PR.

akien-mga · 2020-10-14T13:41:26Z

Thanks!

KoBeWi added enhancement topic:core labels Oct 13, 2020

KoBeWi added this to the 4.0 milestone Oct 13, 2020

Calinou reviewed Oct 13, 2020

View reviewed changes

core/callable_method_pointer.h Outdated Show resolved Hide resolved

core/variant_call.cpp Outdated Show resolved Hide resolved

core/variant_call.cpp Outdated Show resolved Hide resolved

core/variant_call.cpp Show resolved Hide resolved

core/variant_internal.h Outdated Show resolved Hide resolved

fire reviewed Oct 13, 2020

View reviewed changes

core/ustring.cpp Outdated Show resolved Hide resolved

reduz force-pushed the refactor-core-type-bindings branch 4 times, most recently from 5150aeb to 8405443 Compare October 13, 2020 22:15

vnen approved these changes Oct 13, 2020

View reviewed changes

akien-mga reviewed Oct 14, 2020

View reviewed changes

akien-mga force-pushed the refactor-core-type-bindings branch from 8405443 to 328cddb Compare October 14, 2020 08:40

akien-mga force-pushed the refactor-core-type-bindings branch 2 times, most recently from 45f5775 to 1b740fb Compare October 14, 2020 08:44

akien-mga reviewed Oct 14, 2020

View reviewed changes

akien-mga force-pushed the refactor-core-type-bindings branch from 1b740fb to 3f2bba8 Compare October 14, 2020 08:47

akien-mga reviewed Oct 14, 2020

View reviewed changes

reduz force-pushed the refactor-core-type-bindings branch 2 times, most recently from 98c685b to fba1843 Compare October 14, 2020 12:21

akien-mga force-pushed the refactor-core-type-bindings branch from fba1843 to b8c6418 Compare October 14, 2020 13:25

akien-mga merged commit 6053111 into godotengine:master Oct 14, 2020

This was referenced Oct 15, 2020

Refactor MethodBind to use variadic templates #42826

Merged

Parser Error: Cannot find property "rotated" on base "Basis" #42834

Closed

touilleMan mentioned this pull request Oct 17, 2020

Modify Array/Dictionary::operator== to do real key/value comparison #35816

Merged

This was referenced Nov 4, 2020

doc: Sync classref with current source #43310

Merged

Core: disabled method bindings that were not compatible with the new system #43311

Closed

raulsntos mentioned this pull request Oct 7, 2022

C#: Cleanup and sync StringExtensions with core #67031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored binding system for core types #42780

Refactored binding system for core types #42780

reduz commented Oct 13, 2020 •

edited

Loading

vnen left a comment

akien-mga Oct 14, 2020

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020 •

edited

Loading

akien-mga Oct 14, 2020

akien-mga Oct 14, 2020

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020

Refactored binding system for core types #42780

Refactored binding system for core types #42780

Conversation

reduz commented Oct 13, 2020 • edited Loading

vnen left a comment

Choose a reason for hiding this comment

akien-mga Oct 14, 2020

Choose a reason for hiding this comment

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020 • edited Loading

akien-mga Oct 14, 2020

Choose a reason for hiding this comment

akien-mga Oct 14, 2020

Choose a reason for hiding this comment

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020

akien-mga commented Oct 14, 2020

reduz commented Oct 13, 2020 •

edited

Loading

akien-mga commented Oct 14, 2020 •

edited

Loading