Fix create instance func

godotengine / godot-cpp

C++ bindings for the Godot script API

MIT License

1.74k stars 575 forks source link

Fix create instance func #1431

Closed pupil1337 closed 6 months ago

pupil1337 commented 7 months ago

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.

dsnopek commented 7 months ago

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().");

pupil1337 commented 7 months ago

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 commented 7 months ago

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 https://github.com/godotengine/godot-cpp/pull/1435) we want to encourage developers to use the macros.

dsnopek commented 6 months ago

Congrats on your first merged Godot contribution :tada:

pupil1337 commented 6 months ago

Thanks🌹

dsnopek commented 6 months ago

Cherry-picked for 4.2 in PR https://github.com/godotengine/godot-cpp/pull/1465

dsnopek commented 6 months ago

Cherry-picked for 4.1 in PR https://github.com/godotengine/godot-cpp/pull/1466