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

Use struct instead of class for core structures #880

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

aaronfranke
Copy link
Member

This PR does three things:

  • Change class to struct for the core data structures and adjusts the bindings generator to handle this.
  • Add _NO_DISCARD_ to core data structures to match the engine.
  • Remove _native_ptr from core data structures to match the engine (it's just a void pointer cast).

Why change class to struct? Here are several reasons:

  • It matches what the core engine uses.
  • struct's default visibility is public, which all members are now that _native_ptr has been removed.
  • class and struct are actually different things on MSVC. With godot-cpp's types defined with class, any forward declarations copied from an engine module like struct Vector2; will not work correctly on MSVC. This is actually something that I discovered after starting work on this PR, and it makes the case for it stronger.
  • It sets the expectation for users that this is a data structure. This point is purely cosmetic.

@aaronfranke aaronfranke added the enhancement This is an enhancement on the current functionality label Oct 5, 2022
@aaronfranke aaronfranke added this to the 4.0 milestone Oct 5, 2022
@akien-mga akien-mga requested a review from a team October 5, 2022 07:48
@akien-mga akien-mga merged commit 50a534b into godotengine:master Oct 5, 2022
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the struct branch October 5, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants