Skip to content

Remove implicit conversions from math types to String, to avoid accidental conversions #107295

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

Merged
merged 1 commit into from
Jun 9, 2025

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Jun 9, 2025

We've agreed in a previous core meeting that implicit conversions should be reduced in the codebase. As a costly conversion, implicit conversion to String is especially undesirable. This PR makes one small step towards this goal.

As an example, the bug fixed by the above PR (#107289) was only possible because Color had an implicit conversion to String. This PR makes this conversion explicit (among others), preventing similar bugs from happening again in the future.

@Ivorforce Ivorforce added this to the 4.x milestone Jun 9, 2025
@Ivorforce Ivorforce requested a review from a team as a code owner June 9, 2025 00:02
@Ivorforce Ivorforce requested review from a team as code owners June 9, 2025 00:02
@Ivorforce Ivorforce changed the title Make conversions from math types to String explicit, to avoid accidental conversions Remove implicit conversions from math types to String, to avoid accidental conversions Jun 9, 2025
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This code looks good to me, and I agree it's a good idea to make these explicit.

@lawnjelly
Copy link
Member

lawnjelly commented Jun 9, 2025

Change looks well worth making. Incidentally, I don't know if this would work too, but in a similar library recently I put all these string conversion functions into the String class rather than the classes themselves, to avoid bloating the classes with such details.

Now that it is explicit, would an alternative be a constructor on String? 🤔
I guess the choice is make the classes themselves heavier, or the String. There are probably pluses and minuses for both.

class Vector3;

class String
{
public:
	explicit String(int64_t p_value);
	explicit String(const Vector3 &p_value);
	...etc

@Ivorforce
Copy link
Member Author

Ivorforce commented Jun 9, 2025

Now that it is explicit, would an alternative be a constructor on String? 🤔

This is possible, whether they're explicit or implicit.

The main question is which of the types the string conversion binds more tightly to. I'd say String is the more fundamental type compared to any of these math types, so it should not be including them or handling string conversions for them. But i could see other people having a different opinion about this 😄

@lawnjelly
Copy link
Member

The main question is which of the types the string conversion binds more tightly to. I'd say String is the more fundamental type compared to any of these math types, so it should not be including them or handling string conversions for them. But i could see other people having a different opinion about this

Yeah, it's a glass half full / half empty kind of argument, and how you prefer dependencies.
Regardless the PR currently is an improvement. I just thought it worth mentioning this possibility. 👍

@Repiteo Repiteo modified the milestones: 4.x, 4.5 Jun 9, 2025
@Repiteo Repiteo merged commit 7d5ecc2 into godotengine:master Jun 9, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 9, 2025

Thanks!

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