Skip to content

Commit

Permalink
Fix -Wmaybe-uninitialized warning
Browse files Browse the repository at this point in the history
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 <ahans@users.noreply.github.com>
  • Loading branch information
ahans committed Jan 12, 2023
1 parent 1bbb033 commit 70e222e
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 66 deletions.
8 changes: 5 additions & 3 deletions rclcpp/include/rclcpp/context.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -398,20 +398,22 @@ class Context : public std::enable_shared_from_this<Context>

using ShutdownCallback = ShutdownCallbackHandle::ShutdownCallbackType;

template<ShutdownType shutdown_type>
RCLCPP_LOCAL
ShutdownCallbackHandle
add_shutdown_callback(
ShutdownType shutdown_type,
ShutdownCallback callback);

template<ShutdownType shutdown_type>
RCLCPP_LOCAL
bool
remove_shutdown_callback(
ShutdownType shutdown_type,
const ShutdownCallbackHandle & callback_handle);

template<ShutdownType shutdown_type>
RCLCPP_LOCAL
std::vector<rclcpp::Context::ShutdownCallback>
get_shutdown_callback(ShutdownType shutdown_type) const;
get_shutdown_callback() const;
};

/// Return a copy of the list of context shared pointers.
Expand Down
112 changes: 49 additions & 63 deletions rclcpp/src/rclcpp/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,123 +365,109 @@ 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<ShutdownType::on_shutdown>(callback);
}

bool
Context::remove_on_shutdown_callback(const OnShutdownCallbackHandle & callback_handle)
{
return remove_shutdown_callback(ShutdownType::on_shutdown, callback_handle);
return remove_shutdown_callback<ShutdownType::on_shutdown>(callback_handle);
}

rclcpp::PreShutdownCallbackHandle
Context::add_pre_shutdown_callback(PreShutdownCallback callback)
{
return add_shutdown_callback(ShutdownType::pre_shutdown, callback);
return add_shutdown_callback<ShutdownType::pre_shutdown>(callback);
}

bool
Context::remove_pre_shutdown_callback(
const PreShutdownCallbackHandle & callback_handle)
{
return remove_shutdown_callback(ShutdownType::pre_shutdown, callback_handle);
return remove_shutdown_callback<ShutdownType::pre_shutdown>(callback_handle);
}

template<Context::ShutdownType shutdown_type>
rclcpp::ShutdownCallbackHandle
Context::add_shutdown_callback(
ShutdownType shutdown_type,
ShutdownCallback callback)
{
auto callback_shared_ptr =
std::make_shared<ShutdownCallbackHandle::ShutdownCallbackType>(callback);

switch (shutdown_type) {
case ShutdownType::pre_shutdown:
{
std::lock_guard<std::mutex> lock(pre_shutdown_callbacks_mutex_);
pre_shutdown_callbacks_.emplace(callback_shared_ptr);
}
break;
case ShutdownType::on_shutdown:
{
std::lock_guard<std::mutex> 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<std::mutex> lock(pre_shutdown_callbacks_mutex_);
pre_shutdown_callbacks_.emplace(callback_shared_ptr);
} else {
std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
on_shutdown_callbacks_.emplace(callback_shared_ptr);
}

ShutdownCallbackHandle callback_handle;
callback_handle.callback = callback_shared_ptr;
return callback_handle;
}

template<Context::ShutdownType shutdown_type>
bool
Context::remove_shutdown_callback(
ShutdownType shutdown_type,
const ShutdownCallbackHandle & callback_handle)
{
std::mutex * mutex_ptr = nullptr;
std::unordered_set<
std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> * 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<std::mutex> 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<std::mutex> 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<rclcpp::Context::OnShutdownCallback>
Context::get_on_shutdown_callbacks() const
{
return get_shutdown_callback(ShutdownType::on_shutdown);
return get_shutdown_callback<ShutdownType::on_shutdown>();
}

std::vector<rclcpp::Context::PreShutdownCallback>
Context::get_pre_shutdown_callbacks() const
{
return get_shutdown_callback(ShutdownType::pre_shutdown);
return get_shutdown_callback<ShutdownType::pre_shutdown>();
}

template<Context::ShutdownType shutdown_type>
std::vector<rclcpp::Context::ShutdownCallback>
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<ShutdownCallbackHandle::ShutdownCallbackType>> * 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<std::mutex> lock(mutex);
std::vector<rclcpp::Context::ShutdownCallback> callbacks;
for (auto & callback : callback_set) {
callbacks.push_back(*callback);
}
return callbacks;
};

std::vector<rclcpp::Context::ShutdownCallback> callbacks;
{
std::lock_guard<std::mutex> 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<rcl_context_t>
Expand Down

0 comments on commit 70e222e

Please sign in to comment.