From c46896731c3a02b39140bea9d31ae36c99846344 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 2 Aug 2024 08:01:25 -0400 Subject: [PATCH] Make more of the Waitable API abstract (#2593) * Make Waitable::{is_ready,add_to_wait_set,execute} abstract APIs. I initially started looking at this because clang was complaining that "all paths through this function will call itself". And it is correct; if an implementation does not override these methods, and they are every called, they will go into an infinite loop calling themselves. However, while looking at it it seemed to me that these were really part of the API that a concrete implementation of Waitable needed to implement. It is fine if an implementation doesn't want to do anything (like the tests), but all of the "real" users in the codebase override these. Thus, remove the implementations here and make these pure virtual functions that all subclasses must implement. * Remove some more "empty" implementations. In particular, these are implementations in the Waitable class that only throw exceptions. Rather than do this, make these APIs pure virtual so all downstream classes have to implement them. All of the current users (except for tests) do anyway, and it makes the API much cleaner. Signed-off-by: Chris Lalancette --- rclcpp/include/rclcpp/waitable.hpp | 12 ++--- rclcpp/src/rclcpp/waitable.cpp | 46 ------------------- .../node_interfaces/test_node_waitables.cpp | 12 ++--- .../test_allocator_memory_strategy.cpp | 12 ++--- rclcpp/test/rclcpp/test_memory_strategy.cpp | 5 ++ .../test_dynamic_storage.cpp | 10 ++-- .../wait_set_policies/test_static_storage.cpp | 10 ++-- .../test_storage_policy_common.cpp | 9 ++-- .../test_thread_safe_synchronization.cpp | 10 ++-- 9 files changed, 47 insertions(+), 79 deletions(-) diff --git a/rclcpp/include/rclcpp/waitable.hpp b/rclcpp/include/rclcpp/waitable.hpp index 280d79c39d..c803629dcd 100644 --- a/rclcpp/include/rclcpp/waitable.hpp +++ b/rclcpp/include/rclcpp/waitable.hpp @@ -109,7 +109,7 @@ class Waitable RCLCPP_PUBLIC virtual void - add_to_wait_set(rcl_wait_set_t & wait_set); + add_to_wait_set(rcl_wait_set_t & wait_set) = 0; /// Check if the Waitable is ready. /** @@ -124,7 +124,7 @@ class Waitable RCLCPP_PUBLIC virtual bool - is_ready(const rcl_wait_set_t & wait_set); + is_ready(const rcl_wait_set_t & wait_set) = 0; /// Take the data so that it can be consumed with `execute`. /** @@ -176,7 +176,7 @@ class Waitable RCLCPP_PUBLIC virtual std::shared_ptr - take_data_by_entity_id(size_t id); + take_data_by_entity_id(size_t id) = 0; /// Execute data that is passed in. /** @@ -203,7 +203,7 @@ class Waitable RCLCPP_PUBLIC virtual void - execute(const std::shared_ptr & data); + execute(const std::shared_ptr & data) = 0; /// Exchange the "in use by wait set" state for this timer. /** @@ -246,7 +246,7 @@ class Waitable RCLCPP_PUBLIC virtual void - set_on_ready_callback(std::function callback); + set_on_ready_callback(std::function callback) = 0; /// Unset any callback registered via set_on_ready_callback. /** @@ -256,7 +256,7 @@ class Waitable RCLCPP_PUBLIC virtual void - clear_on_ready_callback(); + clear_on_ready_callback() = 0; private: std::atomic in_use_by_wait_set_{false}; diff --git a/rclcpp/src/rclcpp/waitable.cpp b/rclcpp/src/rclcpp/waitable.cpp index 1ee4f9073f..ef3a50a8d9 100644 --- a/rclcpp/src/rclcpp/waitable.cpp +++ b/rclcpp/src/rclcpp/waitable.cpp @@ -54,54 +54,8 @@ Waitable::get_number_of_ready_guard_conditions() return 0u; } -std::shared_ptr -Waitable::take_data_by_entity_id(size_t id) -{ - (void)id; - throw std::runtime_error( - "Custom waitables should override take_data_by_entity_id " - "if they want to use it."); -} - bool Waitable::exchange_in_use_by_wait_set_state(bool in_use_state) { return in_use_by_wait_set_.exchange(in_use_state); } - -void -Waitable::set_on_ready_callback(std::function callback) -{ - (void)callback; - - throw std::runtime_error( - "Custom waitables should override set_on_ready_callback " - "if they want to use it."); -} - -void -Waitable::clear_on_ready_callback() -{ - throw std::runtime_error( - "Custom waitables should override clear_on_ready_callback if they " - "want to use it and make sure to call it on the waitable destructor."); -} - -bool -Waitable::is_ready(const rcl_wait_set_t & wait_set) -{ - return this->is_ready(wait_set); -} - -void -Waitable::add_to_wait_set(rcl_wait_set_t & wait_set) -{ - this->add_to_wait_set(wait_set); -} - -void -Waitable::execute(const std::shared_ptr & data) -{ - // note this const cast is only required to support a deprecated function - this->execute(const_cast &>(data)); -} diff --git a/rclcpp/test/rclcpp/node_interfaces/test_node_waitables.cpp b/rclcpp/test/rclcpp/node_interfaces/test_node_waitables.cpp index aa34a71af4..2b6b1092be 100644 --- a/rclcpp/test/rclcpp/node_interfaces/test_node_waitables.cpp +++ b/rclcpp/test/rclcpp/node_interfaces/test_node_waitables.cpp @@ -31,13 +31,13 @@ class TestWaitable : public rclcpp::Waitable void add_to_wait_set(rcl_wait_set_t &) override {} bool is_ready(const rcl_wait_set_t &) override {return false;} - std::shared_ptr - take_data() override - { - return nullptr; - } - + std::shared_ptr take_data() override {return nullptr;} void execute(const std::shared_ptr &) override {} + + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} }; class TestNodeWaitables : public ::testing::Test diff --git a/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp b/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp index 452228645b..27c2711916 100644 --- a/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp +++ b/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp @@ -51,13 +51,13 @@ class TestWaitable : public rclcpp::Waitable return test_waitable_result; } - std::shared_ptr - take_data() override - { - return nullptr; - } - + std::shared_ptr take_data() override {return nullptr;} void execute(const std::shared_ptr &) override {} + + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} }; struct RclWaitSetSizes diff --git a/rclcpp/test/rclcpp/test_memory_strategy.cpp b/rclcpp/test/rclcpp/test_memory_strategy.cpp index 4c166ebb88..7ceb4e7be4 100644 --- a/rclcpp/test/rclcpp/test_memory_strategy.cpp +++ b/rclcpp/test/rclcpp/test_memory_strategy.cpp @@ -40,6 +40,11 @@ class TestWaitable : public rclcpp::Waitable std::shared_ptr take_data() override {return nullptr;} void execute(const std::shared_ptr &) override {} + + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} }; class TestMemoryStrategy : public ::testing::Test diff --git a/rclcpp/test/rclcpp/wait_set_policies/test_dynamic_storage.cpp b/rclcpp/test/rclcpp/wait_set_policies/test_dynamic_storage.cpp index 12bd2f884e..e69d3480d9 100644 --- a/rclcpp/test/rclcpp/wait_set_policies/test_dynamic_storage.cpp +++ b/rclcpp/test/rclcpp/wait_set_policies/test_dynamic_storage.cpp @@ -52,16 +52,18 @@ class TestWaitable : public rclcpp::Waitable : is_ready_(false) {} void add_to_wait_set(rcl_wait_set_t &) override {} - bool is_ready(const rcl_wait_set_t &) override {return is_ready_;} std::shared_ptr take_data() override {return nullptr;} - - void - execute(const std::shared_ptr &) override {} + void execute(const std::shared_ptr &) override {} void set_is_ready(bool value) {is_ready_ = value;} + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} + private: bool is_ready_; }; diff --git a/rclcpp/test/rclcpp/wait_set_policies/test_static_storage.cpp b/rclcpp/test/rclcpp/wait_set_policies/test_static_storage.cpp index f3e94b6aa0..ed3336e123 100644 --- a/rclcpp/test/rclcpp/wait_set_policies/test_static_storage.cpp +++ b/rclcpp/test/rclcpp/wait_set_policies/test_static_storage.cpp @@ -52,16 +52,18 @@ class TestWaitable : public rclcpp::Waitable : is_ready_(false) {} void add_to_wait_set(rcl_wait_set_t &) override {} - bool is_ready(const rcl_wait_set_t &) override {return is_ready_;} std::shared_ptr take_data() override {return nullptr;} - - void - execute(const std::shared_ptr &) override {} + void execute(const std::shared_ptr &) override {} void set_is_ready(bool value) {is_ready_ = value;} + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} + private: bool is_ready_; }; diff --git a/rclcpp/test/rclcpp/wait_set_policies/test_storage_policy_common.cpp b/rclcpp/test/rclcpp/wait_set_policies/test_storage_policy_common.cpp index eaf3a866d1..f7ae12c2f3 100644 --- a/rclcpp/test/rclcpp/wait_set_policies/test_storage_policy_common.cpp +++ b/rclcpp/test/rclcpp/wait_set_policies/test_storage_policy_common.cpp @@ -61,14 +61,17 @@ class TestWaitable : public rclcpp::Waitable bool is_ready(const rcl_wait_set_t &) override {return is_ready_;} std::shared_ptr take_data() override {return nullptr;} - - void - execute(const std::shared_ptr & data) override {(void)data;} + void execute(const std::shared_ptr &) override {} void set_is_ready(bool value) {is_ready_ = value;} void set_add_to_wait_set(bool value) {add_to_wait_set_ = value;} + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} + private: bool is_ready_; bool add_to_wait_set_; diff --git a/rclcpp/test/rclcpp/wait_set_policies/test_thread_safe_synchronization.cpp b/rclcpp/test/rclcpp/wait_set_policies/test_thread_safe_synchronization.cpp index e6a034832b..12c8838188 100644 --- a/rclcpp/test/rclcpp/wait_set_policies/test_thread_safe_synchronization.cpp +++ b/rclcpp/test/rclcpp/wait_set_policies/test_thread_safe_synchronization.cpp @@ -52,16 +52,18 @@ class TestWaitable : public rclcpp::Waitable : is_ready_(false) {} void add_to_wait_set(rcl_wait_set_t &) override {} - bool is_ready(const rcl_wait_set_t &) override {return is_ready_;} std::shared_ptr take_data() override {return nullptr;} - - void - execute(const std::shared_ptr &) override {} + void execute(const std::shared_ptr &) override {} void set_is_ready(bool value) {is_ready_ = value;} + void set_on_ready_callback(std::function) override {} + void clear_on_ready_callback() override {} + + std::shared_ptr take_data_by_entity_id(size_t) override {return nullptr;} + private: bool is_ready_; };