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

Fix MSVC Variant Workaround #100904

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

kiroxas
Copy link
Contributor

@kiroxas kiroxas commented Dec 29, 2024

Fix this issue.

@kiroxas kiroxas requested a review from a team as a code owner December 29, 2024 15:52
@kiroxas kiroxas marked this pull request as draft December 29, 2024 15:52
@AThousandShips AThousandShips changed the title Fixing Variant Workaround Fix MSVC Variant Workaround Dec 29, 2024
@Repiteo Repiteo added this to the 4.x milestone Dec 29, 2024
@kiroxas kiroxas marked this pull request as ready for review December 29, 2024 16:50
@Ivorforce
Copy link
Member

Ivorforce commented Dec 29, 2024

Thanks for addressing the issue!

I can see it's a simple solution - apparently relying on implicit static casts instead of explicit conversions avoids the bug.

Personally, i avoid implicit conversions because i think they make for code that is hard to understand.
Though I confess, since it avoids my odd workaround, it may be better suited than that especially because the implicit converters do exist in the codebase and there is no Godot policy (i know of) to avoid them.

@Repiteo
Copy link
Contributor

Repiteo commented Dec 30, 2024

The amount of implicit conversions in the codebase is proooobably something that should be looked into eventually. A lot of subtle issues crop up from them, but I'm not certain if it's to the extent that it needs anything done about it

@Ivorforce
Copy link
Member

Another thought; does this close #100605 or do we still want to report the bug to MSVC?

@kiroxas
Copy link
Contributor Author

kiroxas commented Dec 31, 2024

Another thought; does this close #100605 or do we still want to report the bug to MSVC?

If this gets merged, I think we should close #100605. However, that doesnt' mean people can not still try to reproduce the issue and report it to MSVC. I have a small godbolt link trying to reproduce the issue, but with no luck yet.

Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

As mentioned above; I'm not super happy about the use of implicit conversions but it's definitely better than what we have now!

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.

Tested locally (MSVC 2022 17.11.3), it works as expected.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Jan 28, 2025
@Repiteo Repiteo merged commit 1aed2f5 into godotengine:master Jan 28, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 28, 2025

Thanks!

@kiroxas kiroxas deleted the fixVariantMSVCWorkaround branch January 28, 2025 15:10
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