Enable various C++ build-time warnings and treat them as errors #340
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
cpp/CMakeLists.txt
Outdated
| @@ -56,7 +56,7 @@ set(UCXX_BUILD_TESTS ${BUILD_TESTS}) | |||
| set(UCXX_BUILD_BENCHMARKS ${BUILD_BENCHMARKS}) | |||
| set(UCXX_BUILD_EXAMPLES ${BUILD_EXAMPLES}) | |||
|
|
|||
| set(UCXX_CXX_FLAGS "") | |||
| set(UCXX_CXX_FLAGS "-Wall" "-Wattributes" "-Werror") | |||
There was a problem hiding this comment.
Other things that might be worthwhile adding -Wextra, -Wsign-conversion.
There was a problem hiding this comment.
Thanks Lawrence for the suggestions. I've extended those in ba26e7e . Additionally I added -Wno-missing-field-initializers this is because we otherwise hit those warnings when initializing many of the UCP structures where we don't (and probably shouldn't, at least not in all cases) fill those values. Alternatively we could use #pragmas to filter them out, but I'm not sure if it's worth it. If you have other better ideas please let me know as well.
pentschev
changed the title
-Wall -Wattributes -Werror in C++ build
cpp/CMakeLists.txt
Outdated
| @@ -56,7 +56,7 @@ set(UCXX_BUILD_TESTS ${BUILD_TESTS}) | |||
| set(UCXX_BUILD_BENCHMARKS ${BUILD_BENCHMARKS}) | |||
| set(UCXX_BUILD_EXAMPLES ${BUILD_EXAMPLES}) | |||
|
|
|||
| set(UCXX_CXX_FLAGS "") | |||
| set(UCXX_CXX_FLAGS "-Wall" "-Wattributes" "-Werror" "-Wextra" "-Wsign-conversion" "-Wno-missing-field-initializers") | |||
There was a problem hiding this comment.
You shouldn't need all the quotes here.
There was a problem hiding this comment.
Thanks for the hint, removed in f974f2c .
| @@ -145,11 +145,11 @@ RequestAm::RequestAm(std::shared_ptr<Component> endpointOrWorker, | |||
| callbackData) | |||
| { | |||
| std::visit(data::dispatch{ | |||
| [this](data::AmSend amSend) { | |||
| [this](data::AmSend) { | |||
There was a problem hiding this comment.
It seems like there are multiple cases where you dispatch to an empty functor, maybe that should be added as a method for simplification?
There was a problem hiding this comment.
Maybe I'm misunderstanding your suggestion, but the empty functors are not (necessarily) exactly the same, their signature changes with the argument type which is used to match the dispatcher, so I don't think this is possible.
There was a problem hiding this comment.
You're right, never mind. What was confusing me is that you're using a visitor pattern where none of the arms actually use the input object (both here and in other parts of this PR), so it looks like you don't need the input object at all, but in fact you do need it for the dispatch itself to be done so you can't drop the function parameter.
There was a problem hiding this comment.
You nailed it, that's exactly why we need it regardless of whether we use it or not, the type carries at a minimum the meaning of the kind of transfer the request will perform.
…to enable-build-warnings-errors
| { | ||
| THROW_FUTURE_NOT_IMPLEMENTED(); | ||
| } | ||
| RequestNotifierWaitState Worker::waitRequestNotifier(uint64_t) { THROW_FUTURE_NOT_IMPLEMENTED(); } |
There was a problem hiding this comment.
Workaround for cpplint/cpplint#131 .
|
/merge |
Enable various C++ build-time warnings to improve code quality and treat them as errors moving forward. Additionally fix all of those entries that are now considered warnings.