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

Even more gcc warning options #38222

Merged
merged 10 commits into from
Feb 24, 2020

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Feb 22, 2020

Summary

SUMMARY: None

Purpose of change

More compiler checks.

Describe the solution

Adding:

  • -Wformat-signedness which looks for e.g. printf("%d", n); where n is unsigned. There was one example of this in the tests.
  • -Wlogical-op which looks for e.g. a & b which should probably have been a && b. No hits on this one, but it seems like a good warning to have.
  • -Wnon-virtual-dtor which looks for classes with virtual functions but non-virtual destructors. Deleting pointers of such type can be UB. There were a few cases; I'm not sure whether any of them were actually being used incorrectly, but unless there's an extreme performance requirement then we should play it safe.
  • -Wredundant-decls which looks for duplicate function declarations. There were a couple of these which essentially amounted to functions being declared rather than the proper header being included.
  • -Wrestrict which looks for violations of the restrict keyword. That only exists in C++ as an extension, I think. In any case, no examples highlighted. (turned out not to be supported by gcc 5.3)
  • -Wunused-macros which looks for unused macros declared in cpp files (of course, it can't know whether macros in headers are unused. It found a few such.
  • -Wzero-as-null-pointer-constant which is another way to encourage use of nullptr. There was one example, in the curses build where clang-tidy hasn't already eliminated all NULLs.

Describe alternatives you've considered

Adding even more warnings!

Testing

Compiled and ran unit tests.

Additional context

There's a good chance that one or both of the Mingw and Android builds will fail, so don't merge until Travis has shown them to be clean.

These don't trigger anything in our code, but are useful gcc warnings to
have enabled.
@kevingranade
Copy link
Member

src/game.cpp:6841:9: error: macro is not used [-Werror,-Wunused-macros]

#define MAXIMUM_ZOOM_LEVEL 4

    ^

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments labels Feb 22, 2020
@ZhilkinSerg
Copy link
Contributor

ZhilkinSerg commented Feb 22, 2020

 In file included from tools/format/format.cpp:3:0:
tools/format/getpost.h: In function ‘std::__cxx11::string urlDecode(std::__cxx11::string)’:
tools/format/getpost.h:47:67: error: zero as null pointer constant [-Werror=zero-as-null-pointer-constant]
                 tmpchar = static_cast<char>( strtol( tmp, NULL, 0 ) );
                                                                   ^

jbytheway and others added 7 commits February 22, 2020 07:58
This warning detects classes with virtual functions but a non-virtual
destructor.  Deleting instances through pointers whose type is such a
class is undefined behaviour.

I'm not certain whether any of the classes I've added the virtual
destructors to here actually needed it, but there should be negligible
performance impact, and it's a good warning to have enabled.
Turns out it's not actually supported by gcc 5.3.
@ZhilkinSerg ZhilkinSerg merged commit 1c29e3d into CleverRaven:master Feb 24, 2020
@jbytheway jbytheway deleted the gcc_warning_options branch February 24, 2020 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants