Increase compiler warnings #528
Comments
Add a bunch of warning flags based on an article linked by issue #528. From those, a couple of warning flags were omitted. The `-Wformat=2` was omitted due to the `string_format` method, which generates warnings from the unit tests (and is the only place the method is now called from). Additionally the `-Wmissing-prototypes` flag was removed since it was not understood by GCC-7 nor by GCC-8.
Add a bunch of warning flags based on an article linked by issue #528. From those, a few warning flags were omitted. - `-Wformat=2` due to the `string_format` method, which generates warnings from the unit tests (and is the only place the method is now called from). - `-Wmissing-prototypes` since it was not understood by GCC-7 nor by GCC-8. - `-Wredundant-decls` since this caused problems with Mingw-w64 and SDL.
|
Here's another good list of compiler warnings to enable (and some not) for The Big Three by Jason Turner. For reference: CompilersUse every available and reasonable set of warning options. Some warning options only work with optimizations enabled, or work better the higher the chosen level of optimization is, for example You should use as many compilers as you can for your platform(s). Each compiler implements the standard slightly differently and supporting multiple will help ensure the most portable, most reliable code. GCC / Clang
Consider using
MSVC
Not recommended
GeneralStart with very strict warning settings from the beginning. Trying to raise the warning level after the project is underway can be painful. Consider using the treat warnings as errors setting. |
Add a bunch of warning flags based on an article linked by issue #528. From those, a few warning flags were omitted. - `-Wformat=2` due to the `string_format` method, which generates warnings from the unit tests (and is the only place the method is now called from). - `-Wmissing-prototypes` since it was not understood by GCC-7 nor by GCC-8. - `-Wredundant-decls` since this caused problems with Mingw-w64 and SDL.
|
Bit of an update. The bulk of the recommended flags from the link in the first post were enabled by #539, with a few exceptions:
Additionally, the following flags were partially enabled, with exceptions:
The flag The following miscellaneous fix was made:
For the Clang
Remaining flags that produce warnings are: The complement of that, which enables Updating to Clang-11 (instead of Clang-9), the following additional warning is detected:
|
|
From the GCC Warning Options page, the The Note that Given that list, we may be specifying redundant warning flags, which are already enabled by another group flag. |
|
Going through the list of possible additional warnings in this thread, the following additional flags appear to be potentially useful: The following are already included by |
|
Looking through remaining Clang warnings: Uninteresting:
Largely uninteresting:
Mildly interesting:
Interesting:
|
|
I went through the list of recommended GCC warning flags. I was able to add: For the Clang flags, the following had all warnings fixed: Some of the flags listed for Clang also worked with GCC: Remaining GCC flags that have not been enabled: Remaining Clang flags that still produce warnings: |
|
I did a Clang build using Here's the inverted list that enables all flags that still generate warnings: The first few flags are of little to no interest. Removing those, the remaining flags are: |
|
Found this article that may be worth noting here: Some official documentation on warning levels is here: We currently use warning level 4 ( Individual warnings (with code
Some more documentation: Under the section on "Limitations":
This would seem to imply that That is relevant to the recently merged PR #1119. Though in that case it was removed from the NAS2D project, where it didn't seem to be needed. Meanwhile the unit test project may need such an exclusion to enable Code Analysis. |
|
It may also be helpful to check that we have Security Development Lifecycle (SDL) checks on: |
|
I'll re-iterate that turning on |
|
Out of interest, I thought I'd give Most of them don't look particularly interesting. Some are pretty much just informational. So yeah, doesn't really look to be worth enabling. Mind you, we have config that should exclude external headers from error reporting, so the STL and Microsoft provided code are not really a concern. |
We should strive to increase compiler warning options.
In #510, there were links to some useful external resources for warning flags (GCC):
Edit: Diagnostic flags in Clang
As noted in the links, you very likely do not want to enable all possible warning flags, as some may not make sense for modern code, and some may even be contradictory. Nevertheless, it's hard to know what warnings are available, or applicable to your code, without some way to turn them on. Conveniently Clang provides the
-Weverythingflag for such a purpose.I experimented with Clang by enabling the
-Weverythingoption, and then trying to disable warnings until the project was able to build again without warnings or errors. For each warning seen, I disabled it by adding theno-prefix to the flag name. The resulting command that got the project compiling cleanly again was:Edit: The reverse set of flags, to turn on all the warnings that produce errors, the command is:
Some of those are not of interest. In particular, we explicitly use C++17 features, and so we don't care about code that breaks compatibility with C++98. Other flags however do seem to suggest there are things we could improve upon. In particular, unknown commands in Doxygen comments, abuse of the comma operator, unexpected conversions, missing override declarations, no newlines at end of file, unreachable code, and old style casts.
The last link recommended the following flags.
New:
Redundant:
Existing:
There was also mention of
-Weffc++(based on the book Effective C++), though with the suggestion that it wasn't very good, and was pretty noisy. There was also mention of-Wodr(One Definition Rule), which was recommended.Another flag I encountered while paying around was
-Wunused-function.The text was updated successfully, but these errors were encountered: