From f21d70f1826a8b4e80c0192b39d992d624fb4492 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 13 Oct 2020 20:43:03 -0700 Subject: [PATCH 1/7] Add warnings Signed-off-by: Audrow Nash --- rclcpp_action/CMakeLists.txt | 5 ++++- rclcpp_action/include/rclcpp_action/server.hpp | 10 +++++----- rclcpp_action/test/test_client.cpp | 2 +- rclcpp_action/test/test_types.cpp | 4 ++-- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/rclcpp_action/CMakeLists.txt b/rclcpp_action/CMakeLists.txt index 8b1dfe079c..a5b100e5d6 100644 --- a/rclcpp_action/CMakeLists.txt +++ b/rclcpp_action/CMakeLists.txt @@ -13,7 +13,10 @@ if(NOT CMAKE_CXX_STANDARD) set(CMAKE_CXX_STANDARD 14) endif() if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") - add_compile_options(-Wall -Wextra -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual) + add_compile_options( + -Wall -Wextra -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual + -Wformat=2 -Wconversion -Wshadow -Wsign-conversion -Wcast-qual + ) endif() set(${PROJECT_NAME}_SRCS diff --git a/rclcpp_action/include/rclcpp_action/server.hpp b/rclcpp_action/include/rclcpp_action/server.hpp index 5827eb9d0d..b3871060cd 100644 --- a/rclcpp_action/include/rclcpp_action/server.hpp +++ b/rclcpp_action/include/rclcpp_action/server.hpp @@ -381,31 +381,31 @@ class Server : public ServerBase, public std::enable_shared_from_this> weak_this = this->shared_from_this(); std::function)> on_terminal_state = - [weak_this](const GoalUUID & uuid, std::shared_ptr result_message) + [weak_this](const GoalUUID & goal_uuid, std::shared_ptr result_message) { std::shared_ptr> shared_this = weak_this.lock(); if (!shared_this) { return; } // Send result message to anyone that asked - shared_this->publish_result(uuid, result_message); + shared_this->publish_result(goal_uuid, result_message); // Publish a status message any time a goal handle changes state shared_this->publish_status(); // notify base so it can recalculate the expired goal timer shared_this->notify_goal_terminal_state(); // Delete data now (ServerBase and rcl_action_server_t keep data until goal handle expires) std::lock_guard lock(shared_this->goal_handles_mutex_); - shared_this->goal_handles_.erase(uuid); + shared_this->goal_handles_.erase(goal_uuid); }; std::function on_executing = - [weak_this](const GoalUUID & uuid) + [weak_this](const GoalUUID & goal_uuid) { std::shared_ptr> shared_this = weak_this.lock(); if (!shared_this) { return; } - (void)uuid; + (void)goal_uuid; // Publish a status message any time a goal handle changes state shared_this->publish_status(); }; diff --git a/rclcpp_action/test/test_client.cpp b/rclcpp_action/test/test_client.cpp index 7c55a226ed..fe6b3b13c4 100644 --- a/rclcpp_action/test/test_client.cpp +++ b/rclcpp_action/test/test_client.cpp @@ -132,7 +132,7 @@ class TestClient : public ::testing::Test feedback_message.feedback.sequence.push_back(1); feedback_publisher->publish(feedback_message); client_executor.spin_once(); - for (int i = 1; i < goal_request->goal.order; ++i) { + for (uint32_t i = 1; i < static_cast(goal_request->goal.order); ++i) { feedback_message.feedback.sequence.push_back( feedback_message.feedback.sequence[i] + feedback_message.feedback.sequence[i - 1]); diff --git a/rclcpp_action/test/test_types.cpp b/rclcpp_action/test/test_types.cpp index f981261105..7c652aaad6 100644 --- a/rclcpp_action/test/test_types.cpp +++ b/rclcpp_action/test/test_types.cpp @@ -25,12 +25,12 @@ TEST(TestActionTypes, goal_uuid_to_string) { EXPECT_STREQ("0123456789abcdef", rclcpp_action::to_string(goal_id).c_str()); for (uint8_t i = 0; i < UUID_SIZE; ++i) { - goal_id[i] = 16u + i; + goal_id[i] = static_cast(16u + i); } EXPECT_STREQ("101112131415161718191a1b1c1d1e1f", rclcpp_action::to_string(goal_id).c_str()); for (uint8_t i = 0; i < UUID_SIZE; ++i) { - goal_id[i] = std::numeric_limits::max() - i; + goal_id[i] = static_cast(std::numeric_limits::max() - i); } EXPECT_STREQ("fffefdfcfbfaf9f8f7f6f5f4f3f2f1f0", rclcpp_action::to_string(goal_id).c_str()); } From 855bed61ff3486d7d08429918414aa51806a1449 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Thu, 29 Oct 2020 11:47:18 -0700 Subject: [PATCH 2/7] Simplify for loop in test_client.cpp Signed-off-by: Audrow Nash --- rclcpp_action/test/test_client.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp_action/test/test_client.cpp b/rclcpp_action/test/test_client.cpp index fe6b3b13c4..6c3c53d783 100644 --- a/rclcpp_action/test/test_client.cpp +++ b/rclcpp_action/test/test_client.cpp @@ -132,7 +132,7 @@ class TestClient : public ::testing::Test feedback_message.feedback.sequence.push_back(1); feedback_publisher->publish(feedback_message); client_executor.spin_once(); - for (uint32_t i = 1; i < static_cast(goal_request->goal.order); ++i) { + for (int32_t i = 1; i < goal_request->goal.order; ++i) { feedback_message.feedback.sequence.push_back( feedback_message.feedback.sequence[i] + feedback_message.feedback.sequence[i - 1]); From 703107ff17e76da39a9698991a0e8d6914455407 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Dec 2020 10:01:19 -0800 Subject: [PATCH 3/7] Fix conversion warning in test_client static cast to size_t Signed-off-by: Audrow Nash --- rclcpp_action/test/test_client.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp_action/test/test_client.cpp b/rclcpp_action/test/test_client.cpp index 6c3c53d783..42fcb2c17f 100644 --- a/rclcpp_action/test/test_client.cpp +++ b/rclcpp_action/test/test_client.cpp @@ -132,7 +132,7 @@ class TestClient : public ::testing::Test feedback_message.feedback.sequence.push_back(1); feedback_publisher->publish(feedback_message); client_executor.spin_once(); - for (int32_t i = 1; i < goal_request->goal.order; ++i) { + for (size_t i = 1; i < static_cast(goal_request->goal.order); ++i) { feedback_message.feedback.sequence.push_back( feedback_message.feedback.sequence[i] + feedback_message.feedback.sequence[i - 1]); From 6e99f6e4772781cfc5addf5a0a91aef6817682db Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Dec 2020 10:05:10 -0800 Subject: [PATCH 4/7] Fix new warnings after rebasing on master Signed-off-by: Audrow Nash --- rclcpp_action/src/server.cpp | 6 +++--- .../test/benchmark/benchmark_action_client.cpp | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/rclcpp_action/src/server.cpp b/rclcpp_action/src/server.cpp index 8d37303c7d..bcf89a6d96 100644 --- a/rclcpp_action/src/server.cpp +++ b/rclcpp_action/src/server.cpp @@ -474,10 +474,10 @@ ServerBase::execute_result_request_received(std::shared_ptr & data) if (result_response) { // Send the result now - rcl_ret_t ret = rcl_action_send_result_response( + rcl_ret_t rcl_ret = rcl_action_send_result_response( pimpl_->action_server_.get(), &request_header, result_response.get()); - if (RCL_RET_OK != ret) { - rclcpp::exceptions::throw_from_rcl_error(ret); + if (RCL_RET_OK != rcl_ret) { + rclcpp::exceptions::throw_from_rcl_error(rcl_ret); } } else { // Store the request so it can be responded to later diff --git a/rclcpp_action/test/benchmark/benchmark_action_client.cpp b/rclcpp_action/test/benchmark/benchmark_action_client.cpp index 8d2482faca..dbed28be0e 100644 --- a/rclcpp_action/test/benchmark/benchmark_action_client.cpp +++ b/rclcpp_action/test/benchmark/benchmark_action_client.cpp @@ -92,7 +92,7 @@ class ActionClientPerformanceTest : public PerformanceTest current_goal_handle->succeed(result); return; } - for (int i = 2; i < goal->order; ++i) { + for (size_t i = 2; i < static_cast(goal->order); ++i) { result->sequence[i] = result->sequence[i - 1] + result->sequence[i - 2]; } @@ -310,8 +310,8 @@ BENCHMARK_F(ActionClientPerformanceTest, async_cancel_goal)(benchmark::State & s rclcpp::spin_until_future_complete(node, future_cancel, std::chrono::seconds(1)); auto cancel_response = future_cancel.get(); - using CancelResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; - if (CancelResponse::ERROR_NONE != cancel_response->return_code) { + using CancelActionResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; + if (CancelActionResponse::ERROR_NONE != cancel_response->return_code) { state.SkipWithError("Cancel request did not succeed"); break; } @@ -345,8 +345,8 @@ BENCHMARK_F(ActionClientPerformanceTest, async_cancel_all_goals)(benchmark::Stat rclcpp::spin_until_future_complete(node, future_cancel_all, std::chrono::seconds(1)); auto cancel_response = future_cancel_all.get(); - using CancelResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; - if (CancelResponse::ERROR_NONE != cancel_response->return_code) { + using CancelActionResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; + if (CancelActionResponse::ERROR_NONE != cancel_response->return_code) { state.SkipWithError("Cancel request did not succeed"); break; } From b35ece310428a63db8aab37b29874fba1ff00894 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Dec 2020 11:58:24 -0800 Subject: [PATCH 5/7] Fix shadowing in the benchmark action server Signed-off-by: Audrow Nash --- .../test/benchmark/benchmark_action_server.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/rclcpp_action/test/benchmark/benchmark_action_server.cpp b/rclcpp_action/test/benchmark/benchmark_action_server.cpp index 2259a6851f..3ac7fc5812 100644 --- a/rclcpp_action/test/benchmark/benchmark_action_server.cpp +++ b/rclcpp_action/test/benchmark/benchmark_action_server.cpp @@ -187,8 +187,8 @@ BENCHMARK_F(ActionServerPerformanceTest, action_server_cancel_goal)(benchmark::S rclcpp::spin_until_future_complete(node, future_cancel, std::chrono::seconds(1)); auto cancel_response = future_cancel.get(); - using CancelResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; - if (CancelResponse::ERROR_NONE != cancel_response->return_code) { + using CancelActionResponse = test_msgs::action::Fibonacci::Impl::CancelGoalService::Response; + if (CancelActionResponse::ERROR_NONE != cancel_response->return_code) { state.SkipWithError("Cancel request did not succeed"); break; } @@ -247,12 +247,12 @@ BENCHMARK_F(ActionServerPerformanceTest, action_server_set_success)(benchmark::S // too wide, they at least could agree it was fine. In my testing MSVC errored if goal_order was // not captured, but clang would warn if it was explicitly captured. const auto result = [&]() { - auto result = std::make_shared(); + auto action_result = std::make_shared(); for (int i = 0; i < goal_order; ++i) { // Not the fibonacci sequence, but that's not important to this benchmark - result->sequence.push_back(i); + action_result->sequence.push_back(i); } - return result; + return action_result; } (); reset_heap_counters(); @@ -291,12 +291,12 @@ BENCHMARK_F(ActionServerPerformanceTest, action_server_abort)(benchmark::State & // Capturing with & because MSVC and Clang disagree about how to capture goal_order const auto result = [&]() { - auto result = std::make_shared(); + auto action_result = std::make_shared(); for (int i = 0; i < goal_order; ++i) { // Not the fibonacci sequence, but that's not important to this benchmark - result->sequence.push_back(i); + action_result->sequence.push_back(i); } - return result; + return action_result; } (); reset_heap_counters(); From 334d5427d638444e70144866f68745005a271c21 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Dec 2020 11:59:12 -0800 Subject: [PATCH 6/7] Static cast goal order to size_t Signed-off-by: Audrow Nash --- rclcpp_action/test/benchmark/benchmark_action_client.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp_action/test/benchmark/benchmark_action_client.cpp b/rclcpp_action/test/benchmark/benchmark_action_client.cpp index dbed28be0e..fd46ffcf3c 100644 --- a/rclcpp_action/test/benchmark/benchmark_action_client.cpp +++ b/rclcpp_action/test/benchmark/benchmark_action_client.cpp @@ -14,6 +14,7 @@ #include #include +#include #include "performance_test_fixture/performance_test_fixture.hpp" #include "rclcpp_action/rclcpp_action.hpp" @@ -81,7 +82,7 @@ class ActionClientPerformanceTest : public PerformanceTest // Should be checked by the server above assert(goal->order > 0); - result->sequence.resize(goal->order); + result->sequence.resize(static_cast(goal->order)); result->sequence[0] = 0; if (goal->order == 1) { current_goal_handle->succeed(result); From 551330ac1a82447ef25f8aa8fdcd522a4cd6a847 Mon Sep 17 00:00:00 2001 From: Audrow Nash Date: Tue, 15 Dec 2020 12:32:46 -0800 Subject: [PATCH 7/7] Remove unnecessary include Signed-off-by: Audrow Nash --- rclcpp_action/test/benchmark/benchmark_action_client.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/rclcpp_action/test/benchmark/benchmark_action_client.cpp b/rclcpp_action/test/benchmark/benchmark_action_client.cpp index fd46ffcf3c..72d221883e 100644 --- a/rclcpp_action/test/benchmark/benchmark_action_client.cpp +++ b/rclcpp_action/test/benchmark/benchmark_action_client.cpp @@ -14,7 +14,6 @@ #include #include -#include #include "performance_test_fixture/performance_test_fixture.hpp" #include "rclcpp_action/rclcpp_action.hpp"