Add more default warnings #18
Conversation
|
@GretaCB - would love a review here before merging. |
|
Note: most of the work fixing the errors (that came up from more warnings) was done in #19 where I've added many commit comments for context. |
| # Note: -D_GLIBCXX_USE_CXX11_ABI=0 is needed to support mason packages that are precompiled libs | ||
| # Currently we only depend on a header only library, but this will help avoid issues when more libs are added via mason | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OPTIMIZATION_FLAGS} -Wall -Wextra -pedantic -Wsign-compare -Wconversion -Wshadow") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OPTIMIZATION_FLAGS} -D_GLIBCXX_USE_CXX11_ABI=0 ${DESIRED_WARNINGS}") |
There was a problem hiding this comment.
@springmeyer what is D_GLIBCXX_USE_CXX11_ABI used for? a boolean setting for GCC to use a specific c++ version?
There was a problem hiding this comment.
Nice catch. Did not realize I added this flag in this PR. It should have been there all along (as we make mention of it in the code comment above). More background on it is at https://github.com/mapbox/node-cpp-skel/blob/3f7d2c4bcf3a6162312accbf3554ac17ecbbb407/common.gypi#L6. And https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html. In short: until mason supports building against both "ABIS" we need to this flag to avoid edge case linking errors on different linux versions when using mason binary C++ packages. Currently in gzip-hpp we don't link to any mason packages that are C++, so this is technically not needed. But we put it in hpp-skel to avoid this pitfall for downstream users that might add c++ mason binary packages, and therefore my copy/paste here brought it in.
| @@ -17,9 +17,12 @@ else() | |||
| message("-- Configuring release build") | |||
| endif() | |||
|
|
|||
| # Enable extra warnings to adhere to https://github.com/mapbox/cpp/issues/37 | |||
| set(DESIRED_WARNINGS "-Wall -Wextra -Wconversion -Wunreachable-code -Wuninitialized -pedantic-errors -Wold-style-cast -Wno-error=unused-variable -Wshadow -Wfloat-equal") | |||
There was a problem hiding this comment.
These represent 🍇 from mapbox/cpp#37 ?
There was a problem hiding this comment.
Yes, 🍇 and 🍊 (although I now realize I missed -Weffc++)
| @@ -31,7 +33,7 @@ std::string compress(const char* data, | |||
| { | |||
| throw std::runtime_error("deflate init failed"); | |||
| } | |||
| deflate_s.next_in = (Bytef*)data; | |||
| deflate_s.next_in = reinterpret_cast<z_const Bytef*>(data); | |||
There was a problem hiding this comment.
was(Bytef*)data flagged as an error?
There was a problem hiding this comment.
yes, #19 (comment)
| @@ -0,0 +1,5 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
What is the vision of this file?
There was a problem hiding this comment.
One single place to define ZLIB_CONST (without this it would need to be defined in every header before zlib.h is included. This consolidates enabling the const type inside zlib in one place.
| @@ -1,4 +1,7 @@ | |||
| #include <cstdlib> | |||
There was a problem hiding this comment.
Read http://www.cplusplus.com/reference/cstdlib/, but still not sure why this header is needed here
There was a problem hiding this comment.
Needed for uint8_t. Without it, this header would not compile on its own. So this is an IWYU fix, aka include what you use.
There was a problem hiding this comment.
More details on that at https://github.com/include-what-you-use/include-what-you-use. clang-tidy can also flag when headers are depending on types but not including the header for them.
There was a problem hiding this comment.
Is include-what-you-use automatically included in clang-tidy? Or are you using it here as a general good practice?
Also, is tidy succeeding for you locally? I'm getting errors:
/Users/carol/dev/gzip-hpp/mason_packages/osx-x86_64/benchmark/1.2.0/include/benchmark/benchmark.h:832:3: error: Potential memory leak [clang-analyzer-cplusplus.NewDeleteLeaks,-warnings-as-errors]
return internal::RegisterBenchmarkInternal(
^
/Users/carol/dev/gzip-hpp/bench/run.cpp:47:5: note: Taking false branch
if (!stream.is_open())
^
Suppressed 10135 warnings (10134 in non-user code, 1 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
21620 warnings generated.
/Users/carol/dev/gzip-hpp/test/test.cpp:28:5: error: consider replacing 'unsigned long' with 'uint64' [google-runtime-int,-warnings-as-errors]
unsigned long l = 2000000001;
^
Suppressed 21619 warnings (21619 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
Applying fixes ...
|
Sweet, thanks for running this out @springmeyer . Curious if these default flags should be added to hpp-skel as well |
|
Thanks for the review @GretaCB - will merge now.
Yes: mapbox/hpp-skel#52 |
Starts adhering to mapbox/cpp#37