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

Fix create instance func #1431

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

pupil1337
Copy link
Contributor

To Fix #1425

This PR is a resubmission of #1427 , as the repository submitted that time was my master branch it looked bad. So I want to resubmit it.

@pupil1337 pupil1337 requested a review from a team as a code owner April 7, 2024 13:20
@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality discussion topic:buildsystem Related to the buildsystem or CI setup labels Apr 7, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Apr 7, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 10, 2024

Thanks!

The goal here sounds great: cause a compilation error if trying to register an abstract class with GDREGISTER_CLASS() (rather than GDREGISTER_ABSTRACT_CLASS()).

However, I think the implementation here will be confusing for developers (the error they get when using the wrong macros doesn't point to the solution) and for maintainers (my brain has trouble with the !std::is_abstract_v<T> || !is_abstract condition :-)).

What if we added another static_assert() to the top of ClassDB::_register_class() instead?

Something like:

	static_assert(std::is_abstract_V<T> && !is_abstract, "Abstract classes must be registered using GDREGISTER_ABSTRACT_CLASS().");

@dsnopek dsnopek removed the topic:buildsystem Related to the buildsystem or CI setup label Apr 10, 2024
@pupil1337
Copy link
Contributor Author

Thanks!

The goal here sounds great: cause a compilation error if trying to register an abstract class with GDREGISTER_CLASS() (rather than GDREGISTER_ABSTRACT_CLASS()).

However, I think the implementation here will be confusing for developers (the error they get when using the wrong macros doesn't point to the solution) and for maintainers (my brain has trouble with the !std::is_abstract_v<T> || !is_abstract condition :-)).

What if we added another static_assert() to the top of ClassDB::_register_class() instead?

Something like:

	static_assert(std::is_abstract_V<T> && !is_abstract, "Abstract classes must be registered using GDREGISTER_ABSTRACT_CLASS().");

Thank you for your suggestion, it's very useful! I had change last commit.

@dsnopek
Copy link
Collaborator

dsnopek commented Apr 10, 2024

Thanks!

Two additional notes:

  • I think this should be in ClassDB::_register_class() (with a leading underscore) rather than ClassDB::register_class(), because otherwise we have this same problem with GDREGISTER_RUNTIME_CLASS() or GDREGISTER_INTERNAL_CLASS().
  • In the error message, we should only recommend the macro (GDREGISTER_CLASS()) not the function. Despite using the functions directly in the test code (which will be fixed soon by PR Use GDREGISTER defines in example #1435) we want to encourage developers to use the macros.

@@ -203,6 +203,7 @@ class ClassDB {
template <typename T, bool is_abstract>
void ClassDB::_register_class(bool p_virtual, bool p_exposed, bool p_runtime) {
static_assert(TypesAreSame<typename T::self_type, T>::value, "Class not declared properly, please use GDCLASS.");
static_assert(!std::is_abstract_v<T> || is_abstract, "Class is abstract, please use GDREGISTER_ABSTRACT_CLASS.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • if this class is real a no-abstract class, not check, so it can register by GDREGISTER_CLASS GDREGISTER_VIRTUAL_CLASS GDREGISTER_ABSTRACT_CLASS GDREGISTER_INTERNAL_CLASS GDREGISTER_RUNTIME_CLASS etc.. (all register methods)
  • and if this class is real an abstract class, check is_abstract param, make this class must register by GDREGISTER_ABSTRACT_CLASS

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great to me now!

@dsnopek dsnopek merged commit e23b117 into godotengine:master Apr 24, 2024
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 24, 2024

Congrats on your first merged Godot contribution 🎉

@pupil1337
Copy link
Contributor Author

Thanks🌹

@dsnopek
Copy link
Collaborator

dsnopek commented May 17, 2024

Cherry-picked for 4.2 in PR #1465

@dsnopek
Copy link
Collaborator

dsnopek commented May 17, 2024

Cherry-picked for 4.1 in PR #1466

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 22, 2024
@pupil1337 pupil1337 deleted the fix-create-instance-func branch August 26, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When register a Abstract Class using GDREGISTER_CLASS(), compiled without error
4 participants