Skip to content

Fix MSVC warning for potential mod by 0 (C4724) #106634

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 6, 2025

Conversation

nukethebees
Copy link
Contributor

With dev_mode=yes, MSVC warns that some modulo expressions may be x % 0 and refuses to compile.

By assigning the size variables to local variables, the compiler knows size > 0 if the loop bodies are entered. Without the local variable, it's technically possible for size() to become zero after entering the body.

https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4724?view=msvc-170

@nukethebees nukethebees requested a review from a team as a code owner May 20, 2025 12:48
@akien-mga akien-mga added this to the 4.5 milestone May 20, 2025
@akien-mga akien-mga changed the title Fixed MSVC warning for potential mod by 0 (C4724) Fix MSVC warning for potential mod by 0 (C4724) May 20, 2025
@nukethebees
Copy link
Contributor Author

nukethebees commented May 20, 2025

I'm adding static casts to int to fix the failing builds. Will commit soon.

@akien-mga
Copy link
Member

Vector::size returns int64_t, I don't think you need an explicit cast here. We usually just use sizes like this:

const int size = vector.size();

@nukethebees
Copy link
Contributor Author

Vector::size returns int64_t, I don't think you need an explicit cast here. We usually just use sizes like this:

const int size = vector.size();

Sorry, I didn't read the return type closely enough. I'm used to container size types always being unsigned e.g. std::size_t.

Either way, the original usage of uint64_t was wrong and the current cast makes the implicit conversion explicit. I'll fix the incorrect commit message.

@akien-mga
Copy link
Member

We don't usually use explicit static casts in such cases. Our consensus is that it makes code harder to read and doesn't add much value.

@nukethebees
Copy link
Contributor Author

We don't usually use explicit static casts in such cases. Our consensus is that it makes code harder to read and doesn't add much value.

Should I remove the casts or just leave them?

@akien-mga
Copy link
Member

I would just use const int points_size = points.size(); if it works.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@akien-mga akien-mga merged commit 37559a1 into godotengine:master Jun 6, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Zylann
Copy link
Contributor

Zylann commented Jun 27, 2025

So the strange thing is, now my Godot 4.4.1-stable builds are failing with that exact error, warning C4724: potential mod by 0 in cowdata, I wonder what happened because my builds were working before, and now they don't, despite the fact I havent changed the Godot version my module compiles with.

@akien-mga akien-mga added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Jun 27, 2025
@akien-mga
Copy link
Member

Most likely the GitHub Actions runner for Windows was updated to a newer version of MSVC which triggers this warning. @nukethebees might have been running a preview or just a more up-to-date version than GHA a month ago.

@aaronfranke aaronfranke removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 5, 2025
@aaronfranke
Copy link
Member

There were conflicts when backporting this to Godot 4.3, so I opened PR #108321 to manually backport it.

However, this PR can be automatically cherry-picked for Godot 4.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release enhancement topic:codestyle topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants