From 70e222e27e58c687f01b83224fbbde46efa0d65d Mon Sep 17 00:00:00 2001 From: Alexander Hans Date: Thu, 12 Jan 2023 23:23:05 +0100 Subject: [PATCH] Fix -Wmaybe-uninitialized warning gcc 12 warned about `callback_list_ptr` potentially being uninitialized when compiling in release mode (i.e., with `-O2`). Since `shutdown_type` is a compile-time parameter, we fix the warning by enforcing the decision at compile time. Signed-off-by: Alexander Hans --- rclcpp/include/rclcpp/context.hpp | 8 ++- rclcpp/src/rclcpp/context.cpp | 112 +++++++++++++----------------- 2 files changed, 54 insertions(+), 66 deletions(-) diff --git a/rclcpp/include/rclcpp/context.hpp b/rclcpp/include/rclcpp/context.hpp index 3add3f6d46..2917ce3363 100644 --- a/rclcpp/include/rclcpp/context.hpp +++ b/rclcpp/include/rclcpp/context.hpp @@ -398,20 +398,22 @@ class Context : public std::enable_shared_from_this using ShutdownCallback = ShutdownCallbackHandle::ShutdownCallbackType; + template RCLCPP_LOCAL ShutdownCallbackHandle add_shutdown_callback( - ShutdownType shutdown_type, ShutdownCallback callback); + template RCLCPP_LOCAL bool remove_shutdown_callback( - ShutdownType shutdown_type, const ShutdownCallbackHandle & callback_handle); + template + RCLCPP_LOCAL std::vector - get_shutdown_callback(ShutdownType shutdown_type) const; + get_shutdown_callback() const; }; /// Return a copy of the list of context shared pointers. diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index 1599172e2e..e6b11d2c2a 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -365,49 +365,45 @@ Context::on_shutdown(OnShutdownCallback callback) rclcpp::OnShutdownCallbackHandle Context::add_on_shutdown_callback(OnShutdownCallback callback) { - return add_shutdown_callback(ShutdownType::on_shutdown, callback); + return add_shutdown_callback(callback); } bool Context::remove_on_shutdown_callback(const OnShutdownCallbackHandle & callback_handle) { - return remove_shutdown_callback(ShutdownType::on_shutdown, callback_handle); + return remove_shutdown_callback(callback_handle); } rclcpp::PreShutdownCallbackHandle Context::add_pre_shutdown_callback(PreShutdownCallback callback) { - return add_shutdown_callback(ShutdownType::pre_shutdown, callback); + return add_shutdown_callback(callback); } bool Context::remove_pre_shutdown_callback( const PreShutdownCallbackHandle & callback_handle) { - return remove_shutdown_callback(ShutdownType::pre_shutdown, callback_handle); + return remove_shutdown_callback(callback_handle); } +template rclcpp::ShutdownCallbackHandle Context::add_shutdown_callback( - ShutdownType shutdown_type, ShutdownCallback callback) { auto callback_shared_ptr = std::make_shared(callback); - switch (shutdown_type) { - case ShutdownType::pre_shutdown: - { - std::lock_guard lock(pre_shutdown_callbacks_mutex_); - pre_shutdown_callbacks_.emplace(callback_shared_ptr); - } - break; - case ShutdownType::on_shutdown: - { - std::lock_guard lock(on_shutdown_callbacks_mutex_); - on_shutdown_callbacks_.emplace(callback_shared_ptr); - } - break; + static_assert( + shutdown_type == ShutdownType::pre_shutdown || shutdown_type == ShutdownType::on_shutdown); + + if constexpr (shutdown_type == ShutdownType::pre_shutdown) { + std::lock_guard lock(pre_shutdown_callbacks_mutex_); + pre_shutdown_callbacks_.emplace(callback_shared_ptr); + } else { + std::lock_guard lock(on_shutdown_callbacks_mutex_); + on_shutdown_callbacks_.emplace(callback_shared_ptr); } ShutdownCallbackHandle callback_handle; @@ -415,73 +411,63 @@ Context::add_shutdown_callback( return callback_handle; } +template bool Context::remove_shutdown_callback( - ShutdownType shutdown_type, const ShutdownCallbackHandle & callback_handle) { - std::mutex * mutex_ptr = nullptr; - std::unordered_set< - std::shared_ptr> * callback_list_ptr; - - switch (shutdown_type) { - case ShutdownType::pre_shutdown: - mutex_ptr = &pre_shutdown_callbacks_mutex_; - callback_list_ptr = &pre_shutdown_callbacks_; - break; - case ShutdownType::on_shutdown: - mutex_ptr = &on_shutdown_callbacks_mutex_; - callback_list_ptr = &on_shutdown_callbacks_; - break; - } + const auto remove_callback = [this, &callback_handle](auto & mutex, auto & callback_set) { + const std::lock_guard lock(mutex); + const auto callback_shared_ptr = callback_handle.callback.lock(); + if (callback_shared_ptr == nullptr) { + return false; + } + return callback_set.erase(callback_shared_ptr) == 1; + }; - std::lock_guard lock(*mutex_ptr); - auto callback_shared_ptr = callback_handle.callback.lock(); - if (callback_shared_ptr == nullptr) { - return false; + static_assert( + shutdown_type == ShutdownType::pre_shutdown || shutdown_type == ShutdownType::on_shutdown); + + if constexpr (shutdown_type == ShutdownType::pre_shutdown) { + return remove_callback(pre_shutdown_callbacks_mutex_, pre_shutdown_callbacks_); + } else { + return remove_callback(on_shutdown_callbacks_mutex_, on_shutdown_callbacks_); } - return callback_list_ptr->erase(callback_shared_ptr) == 1; } std::vector Context::get_on_shutdown_callbacks() const { - return get_shutdown_callback(ShutdownType::on_shutdown); + return get_shutdown_callback(); } std::vector Context::get_pre_shutdown_callbacks() const { - return get_shutdown_callback(ShutdownType::pre_shutdown); + return get_shutdown_callback(); } +template std::vector -Context::get_shutdown_callback(ShutdownType shutdown_type) const +Context::get_shutdown_callback() const { - std::mutex * mutex_ptr = nullptr; - const std::unordered_set< - std::shared_ptr> * callback_list_ptr; - - switch (shutdown_type) { - case ShutdownType::pre_shutdown: - mutex_ptr = &pre_shutdown_callbacks_mutex_; - callback_list_ptr = &pre_shutdown_callbacks_; - break; - case ShutdownType::on_shutdown: - mutex_ptr = &on_shutdown_callbacks_mutex_; - callback_list_ptr = &on_shutdown_callbacks_; - break; - } + const auto get_callback_vector = [this](auto & mutex, auto & callback_set) { + const std::lock_guard lock(mutex); + std::vector callbacks; + for (auto & callback : callback_set) { + callbacks.push_back(*callback); + } + return callbacks; + }; - std::vector callbacks; - { - std::lock_guard lock(*mutex_ptr); - for (auto & iter : *callback_list_ptr) { - callbacks.emplace_back(*iter); - } - } + static_assert( + shutdown_type == ShutdownType::pre_shutdown || shutdown_type == ShutdownType::on_shutdown); - return callbacks; + if constexpr (shutdown_type == ShutdownType::pre_shutdown) { + return get_callback_vector(pre_shutdown_callbacks_mutex_, pre_shutdown_callbacks_); + } else { + return get_callback_vector(on_shutdown_callbacks_mutex_, on_shutdown_callbacks_); + } } std::shared_ptr