[Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors #11668
Conversation
merlimat
added
component/c++
type/enhancement
| @@ -644,6 +644,7 @@ std::string Commands::messageType(BaseCommand_Type type) { | |||
| return "END_TXN_ON_SUBSCRIPTION_RESPONSE"; | |||
| break; | |||
| }; | |||
| BOOST_THROW_EXCEPTION(std::logic_error("Invalid BaseCommand enumeration value")); | |||
There was a problem hiding this comment.
I think that here (and in similar cases below) the idea here was to make sure we get a warning/error if we leave out an enum, so that we don't forget to add it.
There was a problem hiding this comment.
Yep, you're quite correct, and I believe that this change keeps that invariant intact. Note that what I didn't do was to add a default: case, which would indeed be undesirable since it would cover all future cases automatically.
This "throw" line should essentially never be hit in any normal circumstance. If any items are added or removed from the enum or any cases removed from this switch, the compiler will emit an error and maintainers must deal with it. The only possible way to get here is to reinterpret_cast some arbitrary value to this enum type, which would cause the switch to drop out and hit this throw statement. Before, this would have been scary undefined behavior, but now we'll get an exception. Turning on -Wall makes this situation a warning that we have to deal with, which is a good thing.
pulsar-client-cpp/CMakeLists.txt
Outdated
| @@ -288,6 +288,10 @@ if (NOT APPLE AND NOT MSVC) | |||
| set(CMAKE_CXX_FLAGS_PYTHON "${CMAKE_CXX_FLAGS}") | |||
| # Hide all non-exported symbols to avoid conflicts | |||
| set(CMAKE_CXX_FLAGS " -fvisibility=hidden -Wl,--exclude-libs,ALL ${CMAKE_CXX_FLAGS}") | |||
| # Turn on color erro messages and show additional help with errors: | |||
| set(CMAKE_CXX_FLAGS " -fdiagnostics-show-option -fdiagnostics-color ${CMAKE_CXX_FLAGS}") | |||
There was a problem hiding this comment.
This might not work with older Gcc or Clang versions.
There was a problem hiding this comment.
Ah yes, good point. I'll find the minimum versions for those and limit them.
|
Note that this PR is not quite ready yet (hence the "draft"). I just wanted to run it through the CI checks and not lose the message I typed up. I hadn't compiled in release mode yet, and there are more warnings generated due to some single-use variables created for |
|
Still working on this... built it in clang and found some more warnings that popped up which are actually potentially serious bugs (dangling references). Trying to minimize the scope of this first PR as much as possible while still getting the builds to pass with |
…as errors. This change enables several key warning flags that prevent common mistakes in C++: -Wall -Wformat-security and -Wvla, as well as ensuring the code won't build if it contains warnings. This will help to keep the code base clean and stable long term. I was also planning to enable "-Wextra" as it contains several helpful warnings, but I thought the changes required to get -Wall enabled were getting a bit huge as-is, so I'm going to split that effort into two PRs. Most of the changes fall into four categories: * The vast majority are fixing class member initialization order warnings. These can lead to real bugs and are important to fix. * Next was unused variables or functions - these were mostly found in the tests * Functions with switches on enum values and no default/fallback case resulting in a code path with no return - just needed exception throw statements. * Finally, I also fixed several misuses of the "static" keyword: when applied to global variables or functions, this actually means that the identifier has "internal linkage", meaning it's not accessible outside the current translation unit. Putting this in a header is almost never what you want to do, but it's a common mistake since the meaning is different when applied to class members. The "inline" keyword is a better choice in these circumstances. Tests are still all passing. [C++] Removing some unnecessary variable type changes I meant to remove from the previous commit. I decided to disable the "signed comparison warning" since there are typically tons of these and they're tricky to fix because you have to go through and change all of your integer types to get them to line up right. Most C++ code bases I've worked on in the past also disable this warning. It's too much pain for too little gain.
I had forgotten to build in release the first time, and it revealed more warnings that needed to be addressed. Finally, I formatted everything with clang-format.
… warnings, a few of which were quite serious. This commit fixes these. I also reformatted the CMakeLists file to use the newer options syntax and better support clang and GCC side by side. This seems to work with recent GCC and clang versions. Will need to further test with older compilers as well.
… allowing a locally installed gtest-parallel to be detected.
oversearch
force-pushed
the
feat/cpp_refactoring
branch
from
63205ca
to
933b012
Compare
…ons used in CentOS7
… when CRC32 is disabled
…rmat that was too new, and it was formatting files differently. Probably should add a better check for that at some point, but this change is already getting out of control.
…se problems with the regular build. Reverting them. Also fixing potential mis-use of tar command in the test service start script.
|
/pulsarbot run-failure-checks |
|
@merlimat I think this is ready for review. I believe the current test failures are due to flakiness and not actual problems, since I can't reproduce them locally in any of the several configurations that I'm testing under (multiple versions of GCC and clang under different Linux distros, one of which is WSL). The current CPP test run failed in the Python tests due to a segfault. I've seen this before, but can't reproduce it. I was looking through the log, and it appears that those tests are running under Python 2.7 in the CI system. Is that intended? Official support for Python 2.x ended last year. The Python tests pass (mostly consistently...) for me under Python 3.6 and 3.9 after I fixed a few of the tests in this change. |
|
You can comment |
Yeah, it makes sense. Upgrading Python from 2 to 3 requires some changes especially about Python functions to support Python3, IMO. I've tried to upgrade Python 2 to 3 before in #9684, but I'm not familiar with Python Functions :( There's also an opinion from @eolivelli in #9684 (comment) I think whether should we give up the support for Python2 still needs a discussion, maybe you can send an email to begin a discussion. |
|
Unfortunately we must support Python 2.7 as many users may still be running on Python 2.x. If you want @oversearch you can start a discussion on dev@pulsar.apache.org about defining a EOL for Python 2.7 support. |
|
Thank you for the replies guys, and the review. I had neglected to check the documentation before posting and hadn't realized that Python 2.7 was still officially supported. I'm sure there's a mountain of legacy Python 2x code out there and can certainly understand the desire to offer continued support for those users. I agree that dropping that support, while perhaps a good idea (given the state of the Python ecosystem these days), is not something to be done lightly. I'll get a rebase for this posted up today. |
| static pulsar::Logger* logger() { \ | ||
| inline pulsar::Logger* logger() { \ |
There was a problem hiding this comment.
We should not change static here to inline. Though inline can also avoid multiple definitions error, it will cause that a random definition is chosen in all translation units. But for each translation unit, we need a dependent definition of logger() because the __FILE__ macro is different.
This change has made logs less readable, for example, see logs in #11760. All log lines' file is ReaderTest because it has chosen logger() definition from ReaderTest.cc for all translation units.
There was a problem hiding this comment.
I've changed it back in my PR #11762, PTAL if you have time.
There was a problem hiding this comment.
@BewareMyPower ahh sorry about that! You're quite right - I didn't notice the filenames acting screwy when I was testing this. GCC was complaining about unused functions in several translation units with this marked "static". I assumed it was something wrong with this definition, but I should have checked them all individually, since it's simply DECLARE_LOG_OBJECT() lines that are never actually used.
If you want I can fix this in a separate PR. I'm planning to get another change together anyway for turning on -WExtra.
There was a problem hiding this comment.
Since I've already added the changes in #11762 , I think we can just wait that PR to be merged.
…arnings as errors (#11668) * [C++] Turning on more compiler warning flags, and enforcing warnings as errors. This change enables several key warning flags that prevent common mistakes in C++: -Wall -Wformat-security and -Wvla, as well as ensuring the code won't build if it contains warnings. This will help to keep the code base clean and stable long term. I was also planning to enable "-Wextra" as it contains several helpful warnings, but I thought the changes required to get -Wall enabled were getting a bit huge as-is, so I'm going to split that effort into two PRs. Most of the changes fall into four categories: * The vast majority are fixing class member initialization order warnings. These can lead to real bugs and are important to fix. * Next was unused variables or functions - these were mostly found in the tests * Functions with switches on enum values and no default/fallback case resulting in a code path with no return - just needed exception throw statements. * Finally, I also fixed several misuses of the "static" keyword: when applied to global variables or functions, this actually means that the identifier has "internal linkage", meaning it's not accessible outside the current translation unit. Putting this in a header is almost never what you want to do, but it's a common mistake since the meaning is different when applied to class members. The "inline" keyword is a better choice in these circumstances. Tests are still all passing. [C++] Removing some unnecessary variable type changes I meant to remove from the previous commit. I decided to disable the "signed comparison warning" since there are typically tons of these and they're tricky to fix because you have to go through and change all of your integer types to get them to line up right. Most C++ code bases I've worked on in the past also disable this warning. It's too much pain for too little gain. * Fixed more warnings and issues arising from the Release mode build. I had forgotten to build in release the first time, and it revealed more warnings that needed to be addressed. Finally, I formatted everything with clang-format. * [C++] Compiling under clang with -Wall generates a slew of additional warnings, a few of which were quite serious. This commit fixes these. I also reformatted the CMakeLists file to use the newer options syntax and better support clang and GCC side by side. This seems to work with recent GCC and clang versions. Will need to further test with older compilers as well. * Improved support for running tests outside the Docker environment, by allowing a locally installed gtest-parallel to be detected. * Fixed two more unit tests * Tweaking the compiler settings to work under the older compiler versions used in CentOS7 * Applying clang-format * Forgot to actually fix the real unused variable warnings in this file when CRC32 is disabled * Realized that I was running "check-format" with a version of clang-format that was too new, and it was formatting files differently. Probably should add a better check for that at some point, but this change is already getting out of control. * Apparently my attempts to allow gtest-parallel on a non-root path cause problems with the regular build. Reverting them. Also fixing potential mis-use of tar command in the test service start script. Co-authored-by: Matteo Merli <mmerli@apache.org> (cherry picked from commit 4e60de6) Solved conflicts by modifing following files: - pulsar-client-cpp/lib/Producer.cc - pulsar-client-cpp/lib/LogUtils.h - pulsar-client-cpp/tests/BinaryLookupServiceTest.cc - pulsar-client-cpp/tests/CustomLoggerTest.cc
…arnings as errors (apache#11668) * [C++] Turning on more compiler warning flags, and enforcing warnings as errors. This change enables several key warning flags that prevent common mistakes in C++: -Wall -Wformat-security and -Wvla, as well as ensuring the code won't build if it contains warnings. This will help to keep the code base clean and stable long term. I was also planning to enable "-Wextra" as it contains several helpful warnings, but I thought the changes required to get -Wall enabled were getting a bit huge as-is, so I'm going to split that effort into two PRs. Most of the changes fall into four categories: * The vast majority are fixing class member initialization order warnings. These can lead to real bugs and are important to fix. * Next was unused variables or functions - these were mostly found in the tests * Functions with switches on enum values and no default/fallback case resulting in a code path with no return - just needed exception throw statements. * Finally, I also fixed several misuses of the "static" keyword: when applied to global variables or functions, this actually means that the identifier has "internal linkage", meaning it's not accessible outside the current translation unit. Putting this in a header is almost never what you want to do, but it's a common mistake since the meaning is different when applied to class members. The "inline" keyword is a better choice in these circumstances. Tests are still all passing. [C++] Removing some unnecessary variable type changes I meant to remove from the previous commit. I decided to disable the "signed comparison warning" since there are typically tons of these and they're tricky to fix because you have to go through and change all of your integer types to get them to line up right. Most C++ code bases I've worked on in the past also disable this warning. It's too much pain for too little gain. * Fixed more warnings and issues arising from the Release mode build. I had forgotten to build in release the first time, and it revealed more warnings that needed to be addressed. Finally, I formatted everything with clang-format. * [C++] Compiling under clang with -Wall generates a slew of additional warnings, a few of which were quite serious. This commit fixes these. I also reformatted the CMakeLists file to use the newer options syntax and better support clang and GCC side by side. This seems to work with recent GCC and clang versions. Will need to further test with older compilers as well. * Improved support for running tests outside the Docker environment, by allowing a locally installed gtest-parallel to be detected. * Fixed two more unit tests * Tweaking the compiler settings to work under the older compiler versions used in CentOS7 * Applying clang-format * Forgot to actually fix the real unused variable warnings in this file when CRC32 is disabled * Realized that I was running "check-format" with a version of clang-format that was too new, and it was formatting files differently. Probably should add a better check for that at some point, but this change is already getting out of control. * Apparently my attempts to allow gtest-parallel on a non-root path cause problems with the regular build. Reverting them. Also fixing potential mis-use of tar command in the test service start script. Co-authored-by: Matteo Merli <mmerli@apache.org>
Master Issue: apache/pulsar-client-cpp#112
Motivation
This change enables several key warning flags that prevent common mistakes in C++: -Wall -Wformat-security and -Wvla, as well as ensuring the code won't build if it contains warnings. This will help to keep the code base clean and stable long term. I was also planning to enable "-Wextra" as it contains several helpful warnings, but I thought the changes required to get -Wall enabled were getting a bit huge as-is, so I'm going to split that effort into two PRs.
Modifications
Most of the changes fall into four categories:
Verifying this change
This change is trivial refactoring to eliminate compiler warnings. There should be no functional change whatsoever. I did modify some of the tests to fix warnings, and added a few additional asserts. The tests are all passing.
Documentation
No doc changes are required