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

Remove VARIANT_ARG* macros #58929

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Mar 9, 2022

  • Very old macros from the time Godot was created.
  • Limited arguments to 5 (then later changed to 8) in many places.
  • They were replaced by C++11 Variadic Templates.
  • Renamed methods that take argument pointers to have a "p" suffix. This was used in some places and not in others, so made it standard.
  • Also added a dereference check for Variant*. Helped catch a couple of bugs.

@reduz reduz requested review from a team as code owners March 9, 2022 14:01
@akien-mga akien-mga added this to the 4.0 milestone Mar 9, 2022
@akien-mga akien-mga added the bug label Mar 9, 2022
@@ -125,7 +111,7 @@ Error MessageQueue::push_set(Object *p_object, const StringName &p_prop, const V
return push_set(p_object->get_instance_id(), p_prop, p_value);
}

Error MessageQueue::push_callable(const Callable &p_callable, const Variant **p_args, int p_argcount, bool p_show_error) {
Error MessageQueue::push_callablep(const Callable &p_callable, const Variant **p_args, int p_argcount, bool p_show_error) {
Copy link
Member

Choose a reason for hiding this comment

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

For now it's fine for compatibility, but eventually we could likely remove the args pointer and just make it push_callable(const Callable &p_callable, bool p_show_error). We can pass args via Callable::bind.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I am honestly not sure how it is supposed to be used, although the unlimited varargs now should resolve most of the original need for it.

core/variant/variant.h Outdated Show resolved Hide resolved
@reduz reduz force-pushed the remove-variant-arg-macros branch from 4577d69 to 933a77a Compare March 9, 2022 14:30
scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the remove-variant-arg-macros branch from 933a77a to 7bbac9a Compare March 9, 2022 16:12
@reduz reduz requested a review from a team as a code owner March 9, 2022 16:12
@reduz reduz force-pushed the remove-variant-arg-macros branch from 7bbac9a to 58360b8 Compare March 9, 2022 16:26
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.

Aside from my last comment, this seems good to merge if it passes CI.

* Very old macros from the time Godot was created.
* Limited arguments to 5 (then later changed to 8) in many places.
* They were replaced by C++11 Variadic Templates.
* Renamed methods that take argument pointers to have a "p" suffix. This was used in some places and not in others, so made it standard.
* Also added a dereference check for Variant*. Helped catch a couple of bugs.
@reduz reduz force-pushed the remove-variant-arg-macros branch from 58360b8 to 21637df Compare March 9, 2022 18:05
@akien-mga akien-mga merged commit 33c907f into godotengine:master Mar 9, 2022
@akien-mga
Copy link
Member

Thanks!

argptrs.write[i] = &op.args[i];
argc++;
Copy link
Member

Choose a reason for hiding this comment

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

Bug here, argc is now always 0. I'll make a PR.

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.

2 participants