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

New callable_mp macro, for signals to call method pointers directly. #36393

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 20, 2020

Implemented the callable_mp macro (simple on appearance and extremely complex under the hood).

Allows signals to call method pointers directly via Callable (using custom data). Should improve performance as well as not requiring internal methods to be registered in order to receive signals.


virtual uint32_t hash() const;
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behold the power of Voodoo below..
Making a straw doll gain life and dance is probably simpler than this.

@kuruk-mm
Copy link
Contributor

kuruk-mm commented Feb 20, 2020

I don't know if there is a debate, but with this signal rework are you going to implement lambdas in GDScript?

#endif

template <class T, class... P, size_t... Is>
void call_with_variant_args_helper(T *p_instance, void (T::*p_method)(P...), const Variant **p_args, Callable::CallError &r_error, IndexSequence<Is...>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one raises a warning with GCC 8:

In file included from editor/inspector_dock.cpp:33:
./core/callable_method_pointer.h: In instantiation of 'void call_with_variant_args_helper(T*, void (T::*)(P ...), const Variant**, Callable::CallError&, IndexSequence<Is ...>) [with T = InspectorDock; P = {}; long unsigned int ...Is = {}]':
./core/callable_method_pointer.h:125:40:   required from 'void call_with_variant_args(T*, void (T::*)(P ...), const Variant**, int, Callable::CallError&) [with T = InspectorDock; P = {}]'
./core/callable_method_pointer.h:141:25:   required from 'void CallableCustomMethodPointer<T, P>::call(const Variant**, int, Variant&, Callable::CallError&) const [with T = InspectorDock; P = {}]'
./core/callable_method_pointer.h:139:15:   required from here
./core/callable_method_pointer.h:96:94: error: parameter 'p_args' set but not used [-Werror=unused-but-set-parameter]
   96 | void call_with_variant_args_helper(T *p_instance, void (T::*p_method)(P...), const Variant **p_args, Callable::CallError &r_error, IndexSequence<Is...>) {
      |         

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up adding a #pragma to selectively disable this warning around this method for GCC.

@reduz thinks that it's likely a false positive, I copied the rationale he exposed in the above comment.

WDYT @marxin?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akien-mga akien-mga force-pushed the callable-method-pointer branch 2 times, most recently from 93396e6 to db38a28 Compare February 21, 2020 11:29
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased to fix style and silence a likely GCC false positive warning, should be good to merge now provided CI passes.

@akien-mga akien-mga force-pushed the callable-method-pointer branch 2 times, most recently from 3fb3625 to a9b2c8e Compare February 21, 2020 12:00
@Calinou
Copy link
Member

Calinou commented Feb 21, 2020

Shouldn't it be CALLABLE_MP (uppercase) since it's a macro? Or did we settle on using lowercase macros from now on?

@akien-mga
Copy link
Member

Shouldn't it be CALLABLE_MP (uppercase) since it's a macro? Or did we settle on using lowercase macros from now on?

14:24 <Akien> reduz: Calinou asks if we shouldn't use uppercase CALLABLE_MP instead of callable_mp. I also wondered about it. No strong opinion either way but I think so far we've mostly used uppercase macros for bindings.
14:26 <Akien> Could also be CallableMP if we want to show that it creates a Callable
14:31 <reduz> as you wish, i think its probably more readable lowercase because its actually a function
14:31 <reduz> otherwise people might misunderstand it to be a class
14:32 <reduz> if its lowercase, you will understand it to be a function. The only reason its a macro is to get the name of the function as a string in debug

@akien-mga akien-mga merged commit 0447d6f into godotengine:master Feb 21, 2020
@akien-mga
Copy link
Member

Thanks!

#if defined(DEBUG_METHODS_ENABLED) && defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-but-set-parameter"
#endif
Copy link
Contributor

@kuruk-mm kuruk-mm Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that could be used indeed. Though in this case it does look like a GCC bug, so we might want to keep the compiler-specific hack, at least until we know more about the issue.
If it turns out that GCC is correct and that other compilers simply don't catch the issue, we can use [[maybe_unused]] indeed for p_args.

akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 23, 2020
Was missed in godotengine#36393 because no `callable_mp()` calls were actually
compiled with `tools=no` in that PR.

Also work around GCC warning that also affects the
`call_with_variant_args_ret_helper` variant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants