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

Compile on build server with warnings as errors #237

Merged
merged 3 commits into from
Dec 9, 2018
Merged

Conversation

jerryct
Copy link
Contributor

@jerryct jerryct commented Dec 7, 2018

No description provided.

@jerryct jerryct changed the title Compile with warnings as errors on build server Compile on build server with warnings as errors Dec 7, 2018
@@ -113,7 +113,7 @@ static std::size_t WriteResponse(struct mg_connection* conn,
return body.size();
}

bool MetricsHandler::handleGet(CivetServer* server,
bool MetricsHandler::handleGet(CivetServer* /*server*/,
Copy link
Collaborator

@gjasny gjasny Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also write std::ignore = server; in the body. I always find the commented out parameters a little bit clumsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the way clang-tidy does it:
https://clang.llvm.org/extra/clang-tidy/checks/misc-unused-parameters.html

So if /* ... */ is too clumsy isn't it then better to omit the name altogether?

I now removed the unused parameter name but I can change it to std::ignore if you think this is better.

CMakeLists.txt Outdated

add_compile_options(
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>>:-Wno-deprecated-declarations>
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.3>>:-Werror>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we guard that with a CMake option that will be enabled for CI? It is usually good practice to not use -Werror on the users compilers by default. Otherwise distributions will have to work-around it to not make their builds explode with newer compilers or uncommon platforms/architectures.

Copy link
Contributor Author

@jerryct jerryct Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CMakeLists.txt Outdated

add_compile_options(
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>>:-Wno-deprecated-declarations>
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.3>>:-Werror>
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.3>>:-Wall>
$<$<AND:$<STREQUAL:$<COMPILE_LANGUAGE>,CXX>,$<CXX_COMPILER_ID:AppleClang>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.3>>:-Wextra>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you limit the warnings to certain compiler versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New compiler versions normally add new warnings to -Wall -Wextra. My rationale is that the current Travis CI uses two specific compilers and especially GCC 4.8 is rather old. So we can only ensure for these two compilers that we don't have any warnings. If we would unconditionally switch on -Wall ... with -Werror a user of this library who might use a newer GCC or Clang version, may run into the problem that he cannot compile this library anymore.

Do you know of a better solution?

Copy link
Contributor Author

@jerryct jerryct Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. If we guard the flags for the CI then the version number can be removed. My initial approach was differently to enable the flags for the user as long as the version is the same with the CI. But the guard is better.

Copy link
Contributor Author

@jerryct jerryct Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the no-deprecation-declarations. Is it ok?

Copy link
Contributor

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments :)

@@ -11,7 +11,7 @@ namespace detail {
/// It's the boundary condition of this serial functions.
///
/// \param seed Not effect.
inline void hash_combine(std::size_t *seed) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to ignore seed in the body instead of remove seed.

@@ -1,4 +1,4 @@
#pragma
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@@ -11,7 +11,7 @@ TEST(HistogramTest, initialize_with_zero) {
Histogram histogram{{}};
auto metric = histogram.Collect();
auto h = metric.histogram;
EXPECT_EQ(h.sample_count, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPECT_EQ(h.sample_count, 0U); ---> EXPECT_EQ(0U, h.sample_count);
IIRC, the 1st argument of the function EXPECT_EQ means the expected value, and the 2nd one is real value.

Copy link
Contributor Author

@jerryct jerryct Dec 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only fixed the compiler warnings and preserve the order as it was.

@@ -21,7 +21,7 @@ TEST(HistogramTest, sample_count) {
histogram.Observe(200);
auto metric = histogram.Collect();
auto h = metric.histogram;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use histogram instead of the local variable h.

Copy link
Contributor Author

@jerryct jerryct Dec 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not related to the commit intention (fixing compiler warnings).

@@ -23,7 +23,7 @@ static std::string GetHostName() {
return hostname;
}

int main(int argc, char** argv) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the 2 parameters?
I think it's unnecessary to do this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argc and argv are not used in the function body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for unused parameters, we should not remove them directly.
Just use IGNORE to ignore them.

@@ -113,8 +113,7 @@ static std::size_t WriteResponse(struct mg_connection* conn,
return body.size();
}

bool MetricsHandler::handleGet(CivetServer* server,
struct mg_connection* conn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unnecessary I think.

Copy link
Contributor Author

@jerryct jerryct Dec 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I enabled warnings. The warning complains about an unused parameter. server is not used in the function body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ignore instead of removing it directly.

@@ -20,8 +20,8 @@ TEST(FamilyTest, labels) {
{{const_label.name, const_label.value}}};
family.Add({{dynamic_label.name, dynamic_label.value}});
auto collected = family.Collect();
ASSERT_GE(collected.size(), 1);
ASSERT_GE(collected.at(0).metric.size(), 1);
ASSERT_GE(collected.size(), 1U);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the literal 1 is ok. It's unnecessary to use 1U.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I enabled warnings. size() returns an unsigned type and the warning complains about a compare between signed and unsigned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see.

@gjasny gjasny merged commit 09f56af into jupp0r:master Dec 9, 2018
@jerryct jerryct deleted the warn branch December 9, 2018 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants