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.

Sign up for GitHub

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

Jump to bottom

Remove unused ucaps header #1004

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Remove unused ucaps header #1004

merged 1 commit into from
Jan 19, 2023

Conversation

asmaloney
Copy link
Contributor

These static structs were being included in every file string.hpp was included in...

Part of #999

@asmaloney
Remove unused ucaps header
67be0c9 
These static structs were being included in every file string.hpp was included in...

Part of godotengine#999
@asmaloney asmaloney requested a review from a team as a code owner January 19, 2023 12:16
@akien-mga akien-mga requested a review from bruvzg January 19, 2023 12:28
@akien-mga akien-mga added the enhancement This is an enhancement on the current functionality label Jan 19, 2023
akien-mga
akien-mga approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems fine, though would like bruvzg to confirm this isn't needed by TextServer extensions.

@asmaloney
Copy link
Contributor Author

👍 If it is, I would suggest doing it differently if possible (i.e. not including it in a header that gets included everywhere).

Also - having static structs in a header like that is generally not a good idea. Do you know the reason for it?

bruvzg
bruvzg approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

It is used by fallback text server, but should not be included for the every String use, and can be replaced by String::to_lower call, so it should be OK to remove it.

@akien-mga akien-mga merged commit 2a9b21e into godotengine:master Jan 19, 2023
@akien-mga
Copy link
Member

Thanks!

@asmaloney asmaloney deleted the remove-unused-ucaps branch January 19, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruvzg bruvzg bruvzg approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asmaloney @akien-mga @bruvzg