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

Add check to ensure registered classes are declared #81020

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Aug 26, 2023

Checks that all classes registered to ClassDB have been properly declared with GDCLASS

The error message fired is pretty clear IMO as it references the method called, like so:

core/object/class_db.h(190): error C2338: static_assert failed: 'Class not declared properly, please use GDCLASS.'
core/register_core_types.cpp(249): note: see reference to function template instantiation 'void ClassDB::register_class<ConfigFile>(bool)' being compiled

@AThousandShips AThousandShips added this to the 4.2 milestone Aug 26, 2023
@AThousandShips AThousandShips requested review from a team as code owners August 26, 2023 17:10
@AThousandShips
Copy link
Member Author

If someone has pointers for what to put in the docs for the XR class I'd appreciate, only added because the class became registered, can be done separately

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 26, 2023

Will also see about adding a similar check to godot-cpp if this method is desired

Edit: Doesn't seem to be an issue in godot-cpp, it fails to compile for classes without the macro regardless

@bitsawer
Copy link
Member

Possibly fixes this too: #73936

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I tested this PR on a big project (The Mirror). Everything still works as before. Also, the editor startup time went from ~9.5 seconds to ~8.5 seconds. Wow!

I'm approving but this is just a "check if it works" review, not a code review.

@BastiaanOlij
Copy link
Contributor

Ok, added some descriptions to include in the documentation XML.

@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips force-pushed the object_register_fix branch 3 times, most recently from d79a65c to 37d307e Compare August 27, 2023 07:20
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
doc/classes/OpenXRInteractionProfileMetaData.xml Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

akien-mga commented Aug 27, 2023

Edit: Moved question on OpenXRInteractionProfileMetaData naming scheme to #81037.

It would also make sense to split the fixes to the OpenXR class and the actual core check in separate commits. (OpenXR changes first, so that the core check passes.)

@AThousandShips
Copy link
Member Author

Can split it off later today when I have time!

@AThousandShips
Copy link
Member Author

Probably isn't as critical for 4.0 now but leaving the label for now, feel free to remove it if it isn't considered significant (it still matters for custom modules, but seeing how 4.0.5 is likely the last 4.0 patch it won't affect core much)

@akien-mga akien-mga requested a review from reduz August 27, 2023 10:20
@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 27, 2023

Tested and works well for 3.x as well so if this gets merged I can open a 3.x version as well to ensure correctness, there's no missed cases in 3.x however based on testing it, and that without this check removing GDCLASS does not cause any compilation errors or runtime errors, at least not detectable by CI

@AThousandShips AThousandShips force-pushed the object_register_fix branch 2 times, most recently from 5c288cc to 63d2f6b Compare August 28, 2023 07:27
@AThousandShips AThousandShips requested review from a team as code owners August 28, 2023 07:27
@AThousandShips AThousandShips added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 28, 2023
Checks that all classes registered to `ClassDB` have been properly
declared with `GDCLASS`
@AThousandShips AThousandShips removed request for a team August 28, 2023 10:53
akien-mga added a commit to akien-mga/godot that referenced this pull request Aug 29, 2023
…ter_fix

Add check to ensure registered classes are declared
@akien-mga akien-mga merged commit 7e083e5 into godotengine:master Aug 29, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the object_register_fix branch August 29, 2023 11:11
@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 29, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

None yet

7 participants