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

Remove CoreStringNames and SceneStringNames #50941

Closed

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Jul 27, 2021

See https://chat.godotengine.org/channel/scripting?msg=ZCRd86Xg7vAtDaNKu

Not certain that this is desired, but CoreStringNames and SceneStringNames aren't used very consistently, several of their StringNames are unused, and they're no longer that useful for caching the StringNames since we have the SNAME() macro. I this will make it less work to implement something better like godotengine/godot-proposals#2152.

@AaronRecord AaronRecord requested review from a team as code owners July 27, 2021 13:18
@AaronRecord AaronRecord marked this pull request as draft July 27, 2021 13:19
@YuriSizov YuriSizov added this to the 4.0 milestone Jul 27, 2021
core/variant/variant.cpp Outdated Show resolved Hide resolved
@AaronRecord AaronRecord force-pushed the remove-corestringnames branch 2 times, most recently from c2b4615 to aa2045a Compare July 27, 2021 13:36
@AaronRecord AaronRecord marked this pull request as ready for review July 27, 2021 13:43
@AaronRecord AaronRecord marked this pull request as draft July 27, 2021 13:43
@AaronRecord AaronRecord marked this pull request as ready for review July 27, 2021 13:45
@AaronRecord AaronRecord force-pushed the remove-corestringnames branch 3 times, most recently from 24c8da6 to 9c25c68 Compare July 27, 2021 14:16
@EricEzaM
Copy link
Contributor

EricEzaM commented Jul 27, 2021

I think part of the idea was to prevent bugs caused spelling errors in the strings... although the StringNames classes were not really used very diligently or consistently throughout the codebase. What about SceneStringNames, should that be removed too? There may be others.

@AaronRecord
Copy link
Contributor Author

I think part of the idea was to prevent bugs caused spelling errors in the strings... although the StringNames classes were not really used very diligently or consistently throughout the codebase

Didn't know about SceneStringNames, if CoreStringNames gets removed then it probably should too...
I like godotengine/godot-proposals#2152 better though, I think the signal names should be local to the class they originate from.

What about SceneStringNames, should that be removed too? There may be others.

From a quick search it seems SceneStringNames is the only other similar class.

@AaronRecord AaronRecord requested review from a team as code owners July 27, 2021 17:26
@AaronRecord AaronRecord changed the title Remove CoreStringNames as it's no longer useful Remove CoreStringNames and SceneStringNames Jul 27, 2021
@AaronRecord
Copy link
Contributor Author

Alright, I removed SceneStringNames too... I was careful to not just do a simple regex replace, but to make sure that the strings actually matched what they were before (I used a regex to check for all StringNames that weren't the same as their variable name).

@akien-mga akien-mga modified the milestones: 4.0, 4.x Sep 8, 2022
@akien-mga akien-mga marked this pull request as draft September 8, 2022 13:00
@akien-mga
Copy link
Member

akien-mga commented Sep 8, 2022

I think this may still make sense but would likely need to be redone.

But we should discuss it a bit more beforehand to make sure we agree on what should be done. SNAME() can in theory replace these classes but there's a valid point to make that it's not necessarily a gain to rely more on strings (and thus potential typos) instead of caching them in one location.

Back when this was discussed we also had some ideas to maybe use a macro that could define this kind of static StringName in the class with an actual C++ identifier so we avoid strings, but reduz wasn't fond of the idea IIRC.

It's not urgent for 4.0, this can be done after the 4.0 release as a general code quality measure.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 6, 2023

Related: #80573 and #81303
After the latter is merged, I plan to replace all strings with their singleton equivalents if available. The advantage of stringname singletons is that they allocate the name only once, which results in smaller binary size and faster initialization time (see #80573).

While the gain is rather minimal, it's still better to use them in place of repeated SNAMEs and plain strings. Though unused singleton names and names used once should be removed, I plan to clean up that too.

@akien-mga
Copy link
Member

Superseded by #91909.

@akien-mga akien-mga closed this May 14, 2024
@akien-mga akien-mga removed this from the 4.x milestone May 14, 2024
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.

5 participants