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

Use compiler warnings #118

Closed
bodand opened this issue Oct 4, 2020 · 3 comments
Closed

Use compiler warnings #118

bodand opened this issue Oct 4, 2020 · 3 comments

Comments

@bodand
Copy link
Contributor

bodand commented Oct 4, 2020

The use of compiler warnings is good practice and they should be enabled for all projects. On GCC and Clang -Wall, -Wextra, and -Wpedantic should be enabled, while /W3 on MSVC.

With this, multiple warnings are generated when compiling:
MSCV:

  • C4530: Basically an MSVC-specific warning that one should add /EHsc to the compile flags. This is a simple CMake script fix.

GCC:

  • -Wmisleading-indentation: In Player.cpp there are a few else cases where the following line is indented with a tab. This is not really a misleading indentation problem and is mostly just a false positive on the compiler's part, but that tab should really be a set of spaces since that function is mostly indented with spaces. (Also the whole project's is completely mixed on tabs vs spaces. IMO as a separate issue this should be fixed.)
  • -Wunused-variable: Player.cpp:539 defines int countLength = 0; which is not used anywhere and should be deleted.

Clang:

  • -Winconsistent-missing-override: Descendants of the Enemy class seem to override virtual member functions such as GetIntro() and others like ReturnDamage() which come from Entity. These functions should be marked override but they are not. I guess the compiler figures out what you wanted to do, but this is really bad practice. Note that this is triggered even if one passes no arguments to clang while compiling, as in this is a default warning.
  • -Wunused-private-field: The Game class's _Level member is not used anywhere and should be deleted. (Also, identifiers starting with an underscore and then an upper case character (like _Level, or _Player) are reserved for implementations, and using them is undefined behavior)
  • -Wreorder-ctor: The member initializer list of SoundMaker class initializes members out of order. This is, iirc, UB, but most certainly nothing fun. Lines 77 and 76 should be swapped.

I can make a PR with these fixes if it is going to be considered.

@tagniam
Copy link
Owner

tagniam commented Oct 4, 2020

... the whole project's is completely mixed on tabs vs spaces. IMO as a separate issue this should be fixed.

... identifiers starting with an underscore and then an upper case character (like _Level, or _Player) are reserved for implementations, and using them is undefined behavior

I will soon be pushing out a code style change that will finally fix these issues once and for all. :) I previously opened an issue (#7) that tried to address this but I closed it due to inactivity.

Feel free to make a PR that fixes all the warnings you listed. As an extension, I would like to use these compiler warnings as part our Travis CI pipeline, let me know if you feel up to the task - otherwise, I'll create a separate issue for it.

@bodand
Copy link
Contributor Author

bodand commented Oct 4, 2020

OK, I'll create the PR in a few minutes.

For the Travis CI pipeline, I have no idea as I never worked with Travis nor any CI really. In the past, I once tried using it, but it didn't really work out, so I can't say I'm up to that task.
Also, I'd look into the new GitHub actions instead, because then you can keep development completely inside GitHub. But again, I never really tried either of them.
Read below

@bodand
Copy link
Contributor Author

bodand commented Oct 4, 2020

Oh, we already have a CI pipeline. I completely missed that. Then the CMake build should work with the warnings. As for detecting them, I have no idea.

@tagniam tagniam closed this as completed Oct 7, 2020
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

No branches or pull requests

2 participants