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

Move singleton StringName definitions to header #99984

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Dec 3, 2024

CoreStringNames, SceneStringNames and EditorStringNames are declared in the header and then defined in cpp file. For no reason actually, because you can define them in place. Since cpp files only have the definitions, moving them to the header allowed deleting the files completely. This has a couple advantages:

  • It's easier to add new names.
  • There is no order mismatch between declarations and definitions.
  • Inlined definitions can be const.

SceneStringNames order was a mess, so I also rearranged it a bit. I also fixed inconsistency in how some methods were implemented.

@KoBeWi KoBeWi added this to the 4.x milestone Dec 3, 2024
@KoBeWi KoBeWi requested review from a team as code owners December 3, 2024 20:42
@KoBeWi KoBeWi force-pushed the negative_diff_but_it's_deleting_whole_files branch 2 times, most recently from 7082e31 to 98a52c6 Compare December 4, 2024 18:54
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Makes sense to me

core/core_string_names.h Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the negative_diff_but_it's_deleting_whole_files branch from 98a52c6 to d3c9bee Compare December 6, 2024 12:43
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 6, 2024
@Repiteo Repiteo merged commit 2654dba into godotengine:master Dec 9, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 9, 2024

Thanks!

@KoBeWi KoBeWi deleted the negative_diff_but_it's_deleting_whole_files branch December 9, 2024 20:39
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.

4 participants