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

Recommendations for enabling more compiler checks #37

Open
springmeyer opened this Issue Oct 12, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@springmeyer
Copy link
Member

springmeyer commented Oct 12, 2017

Compilers enable by default, a set of warnings they will emit on dodgy or less-than-ideal code. These defaults:

  • a. change over time
  • b. vary between compiler versions
  • c. do not include some of the most valuable warnings that might help prevent bugs

This issue is designed to focus on the problem of c from the perspective of the latest clang++ 4.x versions.

Enabling more warnings than compilers do by default can be an important way of catching bugs early. For example integer truncation bugs can often be detected by enabling -Wconversion, which is not on by default.

Enabling more warnings is very difficult to do after a project is big (e.g. Project-OSRM/osrm-backend#4495 and mapnik/mapnik#2907 and mapnik/mapnik#3204). It is best not to wait and rather to start a project with aggressive warnings from the beginning.

So, the question then becomes: what is a good set of aggressive warnings to enable at the start of a project (or to try to integrate into existing projects)?

In particular:

  1. 🍇 Which flags we should always enable for all code no matter what?

  2. 🍊 What additional flags may be very useful in some scenarios/some code bases?

  3. 🍏 For small projects where it is feasible, can we actually start with clang++s -Weverything? This, when feasible, might be ideal. How to do it?

  4. 🍎 What compiler specific flags should we recommend (that only currently work for clang++ or g++)?

@springmeyer

This comment has been minimized.

Copy link
Member Author

springmeyer commented Oct 12, 2017

My proposal for 🍇 is:

Note: all the above flags ARE NOT turned on by default in clang++ (according to https://clang.llvm.org/docs/DiagnosticsReference.html). If they were then there would be no need to list them explicitly.

My proposal for 🍊 is:

  • -Wshadow - Clang++ shadow warnings are useful while GCC tends to spit out a lot of spurious ones. Developers of large code bases that support GCC may wish to not enable this warning. Developers of small code bases should.
  • -Wfloat-equal - Comparing floats should be done with an epsilon. This is ideal when you are starting new code and might be very painful to enable for large codebases. See mapbox/wagyu@737760b and mapnik/mapnik#3357 for context.
  • -Weffc++ - useful when building with g++ (does not do much with clang++). With g++ it can catch uninitialized class members and prevent crashes like mapbox/wagyu#69 - refs mapbox/wagyu#70

My proposal for 🍏 is:

  • -Weverything
  • -Wno-c++98-compat-pedantic
  • -Wno-c++98-compat

🍎 proposal is:

clang++ only

  • -Wmost

g++ only

TODO: develop our recommended list based on testing https://kristerw.blogspot.de/2017/09/useful-gcc-warning-options-not-enabled.html

springmeyer added a commit to mapbox/node-cpp-skel that referenced this issue Oct 12, 2017

springmeyer added a commit to mapbox/node-cpp-skel that referenced this issue Oct 12, 2017

springmeyer added a commit to mapbox/hpp-skel that referenced this issue Oct 12, 2017

springmeyer added a commit to mapbox/gzip-hpp that referenced this issue Oct 12, 2017

springmeyer added a commit to mapbox/gzip-hpp that referenced this issue Oct 12, 2017

@joto

This comment has been minimized.

Copy link

joto commented Oct 13, 2017

One important tip here: All code that is #included from somewhere outside your own code should be included as system library by having the right include path configuration. Use -isystem <DIR> instead of -I <DIR> for this on compiler command lines, in CMake it is include_directories(SYSTEM <DIR>) instead of include_directories(<DIR>). This way warnings are not reported from those libraries. Otherwise you often have to disable warnings for code in libraries you don't have control over. For instance I usually have a copy of catch.hpp in my code for testing, but I put it into its own directory separate from my own headers.

@daniel-j-h

This comment has been minimized.

Copy link
Contributor

daniel-j-h commented Oct 13, 2017

I want to flag the following:

springmeyer added a commit to mapbox/spatial-algorithms that referenced this issue Oct 16, 2017

@springmeyer

This comment has been minimized.

Copy link
Member Author

springmeyer commented Oct 16, 2017

Thanks @joto: Absolutely agree that, to allow more aggressive warnings in your code (and especially -Werror), this must be paired with using -isystem or include_directories(SYSTEM <DIR>) for deps you don't control. As a few examples, I've done this at mapbox/node-cpp-skel#42 and https://github.com/mapbox/hpp-skel/blob/0af4ae29ec83fe12db20963fcd0855dbe7708a57/CMakeLists.txt#L30.

@daniel-j-h - thanks for these links. So far the flags I'm recommending above work in both recent g++ and clang++. Your links raise the issue that it would be useful to also develop recommended lists of additional warnings to enable, per compiler. I will edit inline above to add another section specific to compilers. Then I will plan to PR a doc that will list these and we can iterate on that doc.

artemp added a commit to mapbox/spatial-algorithms that referenced this issue Oct 17, 2017

Merge pull request #14 from mapbox/warnings
enable more warnings/make them errors per mapbox/cpp#37

springmeyer added a commit to mapbox/hpp-skel that referenced this issue Oct 17, 2017

@springmeyer

This comment has been minimized.

Copy link
Member Author

springmeyer commented Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.