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/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..72d221883e 100644 --- a/rclcpp_action/test/benchmark/benchmark_action_client.cpp +++ b/rclcpp_action/test/benchmark/benchmark_action_client.cpp @@ -81,7 +81,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); @@ -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; } 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(); diff --git a/rclcpp_action/test/test_client.cpp b/rclcpp_action/test/test_client.cpp index 7c55a226ed..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 (int 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]); 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()); }