-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add more default warnings #18
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
|
||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @springmeyer what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
if (WERROR) | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
#include <iostream> | ||
#include <gzip/config.hpp> | ||
|
||
// zlib | ||
#include <zlib.h> | ||
// std | ||
#include <limits> | ||
#include <stdexcept> | ||
#include <string> | ||
|
||
namespace gzip { | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah seeing #19 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, #19 (comment) |
||
|
||
#ifdef DEBUG | ||
// Verify if size input will fit into unsigned int, type used for zlib's avail_in | ||
|
@@ -57,7 +59,7 @@ std::string compress(const char* data, | |
// There is no way we see that "increase" would not fit in an unsigned int, | ||
// hence we use static cast here to avoid -Wshorten-64-to-32 error | ||
deflate_s.avail_out = static_cast<unsigned int>(increase); | ||
deflate_s.next_out = (Bytef*)(output.data() + length); | ||
deflate_s.next_out = reinterpret_cast<Bytef*>((&output[0] + length)); | ||
// From http://www.zlib.net/zlib_how.html | ||
// "deflate() has a return value that can indicate errors, yet we do not check it here. | ||
// Why not? Well, it turns out that deflate() can do no wrong here." | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#pragma once | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the vision of this file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One single place to define |
||
|
||
#ifndef ZLIB_CONST | ||
#define ZLIB_CONST | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
#include <cstdlib> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Read http://www.cplusplus.com/reference/cstdlib/, but still not sure why this header is needed here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, seeing #19 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More details on that at https://github.com/include-what-you-use/include-what-you-use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
|
||
namespace gzip { | ||
|
||
// These live in gzip.hpp because it doesnt need to use deps. | ||
// Otherwise, they would need to live in impl files if these methods used | ||
// zlib structures or functions like inflate/deflate) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These represent 🍇 from mapbox/cpp#37 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 🍇 and 🍊 (although I now realize I missed
-Weffc++
)