You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/13 22:48:03 UTC
[GitHub] [pulsar] oversearch opened a new pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
oversearch opened a new pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668
Master Issue: #11632
### 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:
* 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.
*
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-903781872
> Is that intended? Official support for Python 2.x ended last year.
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 https://github.com/apache/pulsar/pull/9684, but I'm not familiar with Python Functions :(
There's also an option from @eolivelli in https://github.com/apache/pulsar/pull/9684#issuecomment-784253625 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-899865518
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 `-Wall` (I was going to do `-Wextra` also, but that was a lot of additional warnings... so putting that off for the next round).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-902073075
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r689686002
##########
File path: pulsar-client-cpp/CMakeLists.txt
##########
@@ -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}")
Review comment:
Ah yes, good point. I'll find the minimum versions for those and limit them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower edited a comment on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-903781872
> Is that intended? Official support for Python 2.x ended last year.
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 https://github.com/apache/pulsar/pull/9684, but I'm not familiar with Python Functions :(
There's also an opinion from @eolivelli in https://github.com/apache/pulsar/pull/9684#issuecomment-784253625 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r689693314
##########
File path: pulsar-client-cpp/lib/Commands.cc
##########
@@ -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"));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-903790772
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r694899772
##########
File path: pulsar-client-cpp/lib/LogUtils.h
##########
@@ -35,7 +35,7 @@ namespace pulsar {
#endif
#define DECLARE_LOG_OBJECT() \
- static pulsar::Logger* logger() { \
+ inline pulsar::Logger* logger() { \
Review 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 https://github.com/apache/pulsar/issues/11760. All log lines' file is `ReaderTest` because it has chosen `logger()` definition from `ReaderTest.cc` for all translation units.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r689012284
##########
File path: pulsar-client-cpp/lib/Commands.cc
##########
@@ -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"));
Review 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.
##########
File path: pulsar-client-cpp/CMakeLists.txt
##########
@@ -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}")
Review comment:
This might not work with older Gcc or Clang versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-903912101
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-903776147
You can comment `/pulsarbot run-failure-checks` to rerun the tests. BTW, please rebase to master to solve the conflict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r694922763
##########
File path: pulsar-client-cpp/lib/LogUtils.h
##########
@@ -35,7 +35,7 @@ namespace pulsar {
#endif
#define DECLARE_LOG_OBJECT() \
- static pulsar::Logger* logger() { \
+ inline pulsar::Logger* logger() { \
Review comment:
I've changed it back in my PR https://github.com/apache/pulsar/pull/11762, PTAL if you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r689719433
##########
File path: pulsar-client-cpp/lib/Commands.cc
##########
@@ -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"));
Review comment:
Got it. Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r695159041
##########
File path: pulsar-client-cpp/lib/LogUtils.h
##########
@@ -35,7 +35,7 @@ namespace pulsar {
#endif
#define DECLARE_LOG_OBJECT() \
- static pulsar::Logger* logger() { \
+ inline pulsar::Logger* logger() { \
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-902026362
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#issuecomment-899656153
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 `assert` statements. I'll have an updated PR ready soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11668: [Issue 11632][C++] Turning on more compiler warnings, and enforcing warnings as errors
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11668:
URL: https://github.com/apache/pulsar/pull/11668#discussion_r695357026
##########
File path: pulsar-client-cpp/lib/LogUtils.h
##########
@@ -35,7 +35,7 @@ namespace pulsar {
#endif
#define DECLARE_LOG_OBJECT() \
- static pulsar::Logger* logger() { \
+ inline pulsar::Logger* logger() { \
Review comment:
Since I've already added the changes in #11762 , I think we can just wait that PR to be merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org