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

[Core] Improve error messages with vformat #98091

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

AThousandShips
Copy link
Member

Limited to cases where the message contained more than one substitution, can expand to all cases in core but these are the more hard to read, keeping it in small batches and starting with this. Also made some minor improvements to the messages like adding ' and trailing periods in some cases to improve readability.

Left the ones in string.cpp alone as they would likely cause recursion since at least some String methods are used in vformat itself.

See: #91521 (comment)

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I approve of the general pattern of replacing string concatenation with vformat format strings.

@AThousandShips
Copy link
Member Author

AThousandShips commented Oct 12, 2024

I'll look at the scope of adding all single case concatenations as well for completeness, and see if I'll just add them to this or do it as a potential future improvement

Edit: They are quite extensive so leaving them for now until this has gotten more approval

@AThousandShips
Copy link
Member Author

Will try some ideas for improving the performance of vformat, similar to my other performance improvements in String, should make it even more relevant to use it for these cases beyond just readability

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@AThousandShips
Copy link
Member Author

Will look at adding the remaining cases with fewer concatenations, but won't have the time today

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Oct 30, 2024
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Won't merge until those other concatenation cases you mentioned are addressed, but everything so far looks good!

@AThousandShips
Copy link
Member Author

Will try to get to them this week

@AThousandShips AThousandShips force-pushed the improve_err_prints_core branch from 59ce671 to afd3029 Compare October 30, 2024 14:55
@AThousandShips AThousandShips force-pushed the improve_err_prints_core branch from afd3029 to 38f9769 Compare October 30, 2024 14:56
@AThousandShips AThousandShips requested review from a team as code owners October 30, 2024 14:56
@AThousandShips
Copy link
Member Author

That covers most cases I belive, left some cases where I'm unsure about the safety, like in ustring.cpp, and also left ip_address.cpp as it didn't include variant.h and leaving it simple

@Repiteo Repiteo merged commit 163bf94 into godotengine:master Nov 1, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 1, 2024

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

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