approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
318 stars 51 forks source link

Turn on compiler warnings to detect code errors #9

Closed claremacrae closed 5 years ago

claremacrae commented 5 years ago

We aren't taking advantage of compiler warnings to spot C++ language problems or areas for improvement in our code.

For example, see the changes in 93d97a8dc19f19c2cefe575bb50f19fb7c34775e - I spotted that after we did a release - it would have been nice to have one of our compilers point it out for us instead.

Lots has been written about the benefits of enabling warnings from C++ compilers, and how to enable this, for example:

https://arne-mertz.de/2016/02/compiler-warnings-treat-them-right/ https://arne-mertz.de/2016/02/compiler-warnings-part-2-tune-the-compiler/

I'm using this ticket to track a few changes in our code and CMake configuration, to see if we can enable treating warnings as errors, at least for the Catch2 tests, which exercise most of the ApprovalTests.cpp library.

claremacrae commented 5 years ago

This is another good resource. https://github.com/lefticus/cppbestpractices/blob/54e2a3b7a46a1918d9b88d7ee7c2b433ab57acdd/02-Use_the_Tools_Available.md#compilers

claremacrae commented 5 years ago

These earlier commits were to start this process:

claremacrae commented 5 years ago

I wonder whether the old version of Clang2 that we are using is suppressing some warnings that we should address:

https://github.com/approvals/ApprovalTests.cpp/blob/master/lib/Catch.hpp#L24

claremacrae commented 5 years ago

I wonder whether the old version of Clang2 that we are using is suppressing some warnings that we should address:

https://github.com/approvals/ApprovalTests.cpp/blob/master/lib/Catch.hpp#L24

Updating to Clang v2.7.1 removed a few warning suppressions - and all our code still compiles fine on Windows and gcc CI builds, and on CLang 6 on my WSL environment....

So I'm closing this as done.