Skip to content

Conversation

@Architector4
Copy link
Contributor

I built the project with -Dwarning_level=2 (-Wall -Wextra) with Clang and went over warnings in a few files.

This doesn't comprehensively clean up all of them, only a few, but also a few tiny logic bugs I noticed. Kind of mostly looking for input on this, because I imagine there might be disagreements to some of those.

For instance:

  • Clang warns about code like if (var = MightReturnNull()) … because the intent might have been to use == instead, and advises to use double parentheses around the function as a way to explicitly denote the intent of assignment. I did that a bunch of times.
  • Clang warns about dividing an integer then casting to float because the other way around incurs less precision loss. This primarily applies to code that computes the center of a bitmap/whatnot with integer coordinates and stores the result to a Vector (which uses floats). I switched those around too.
  • I cleaned a bunch of unused variables, function arguments, and even imported but never used headers. For those I tried paying attention as to which commit has introduced them and if there was any reason they existed at that time, but they pretty much all appear to come from the initial commit unused already I think lol
  • One spot in SceneMan.cpp runs a function that returns a std::vector and then does std::move on that, which had Clang warn Moving a temporary object prevents copy elision and suggest just removing it, so, yeah lol
  • But what if a metagame lasts for more than 99 million rounds?? ...said a Clang warning about GetRoundName using a too small numStr buffer, so I expanded it lol
  • There was also an "definition of implicit copy constructor is deprecated because it has a user-provided copy assignment operator" warning about Vector, so I gave it an explicit copy constructor. There was also a vice-versa of that warning for Timer so I gave it an explicit copy assignment operator lol

With this patch the game still compiles just fine. I played a few scenarios and a metagame and everything seems dandy.

But yeah, looking for input.

@Architector4
Copy link
Contributor Author

oh what the heck

@Architector4
Copy link
Contributor Author

ok now the CI passes

don't ask me on ethics of serializing a size_t ok 👍

Copy link
Contributor

@Causeless Causeless left a comment

Choose a reason for hiding this comment

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

Very nice!

@Causeless Causeless added this pull request to the merge queue Nov 28, 2025
Merged via the queue into cortex-command-community:development with commit 0160b86 Nov 28, 2025
4 checks passed
@Architector4
Copy link
Contributor Author

...I take it as that everything I listed and did here is just fine? lol

@Architector4 Architector4 deleted the patch-cleanup-assortment branch November 28, 2025 21:48
@HeliumAnt
Copy link
Contributor

these are (almost) entirely semantic/style warnings, there's not much you can do wrong. (though technically the float divisions were actually better before since doing it the "correct" way causes subpixel misalignment, which will potentially cause issues at some point)
at the same time imo fixing these is neither harmful nor particularly helpful either. Wextra is ok to use, although the codebase still isn't in a state where those warnings are particularly meaningful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants