Merge pull request #3 from mjeronimo/mjeronimo/address-review-feedback

Pass SharedPtrs callback remove functions instead of bare pointers

rclcpp/include/rclcpp/parameter_event_handler.hpp

@@ -98,12 +98,12 @@ class ParameterEventHandler
 
   /// Remove parameter event callback registered with add_parameter_event_callback.
   /**
-   * \param[in] handle Pointer to the handle for the callback to remove.
+   * \param[in] callback_handle Handle of the callback to remove.
    */
   RCLCPP_PUBLIC
   void
   remove_parameter_event_callback(
-    const ParameterEventCallbackHandle * const handle);
+    ParameterEventCallbackHandle::SharedPtr callback_handle);
 
   using ParameterCallbackType = ParameterCallbackHandle::ParameterCallbackType;
 
@@ -129,12 +129,12 @@ class ParameterEventHandler
    * is erased from the list of callback handles on the {parameter_name, node_name} in the map.
    * An error is thrown if the handle does not exist and/or was already removed.
    *
-   * \param[in] handle Pointer to callback handle to be removed.
+   * \param[in] callback_handle Handle of the callback to remove.
    */
   RCLCPP_PUBLIC
   void
   remove_parameter_callback(
-    const ParameterCallbackHandle * const handle);
+    ParameterCallbackHandle::SharedPtr callback_handle);
 
   /// Get an rclcpp::Parameter from a parameter event.
   /**

rclcpp/src/rclcpp/parameter_event_handler.cpp

@@ -56,13 +56,13 @@ struct HandleCompare
 
 void
 ParameterEventHandler::remove_parameter_event_callback(
-  const ParameterEventCallbackHandle * const handle)
+  ParameterEventCallbackHandle::SharedPtr callback_handle)
 {
   std::lock_guard<std::recursive_mutex> lock(mutex_);
   auto it = std::find_if(
     event_callbacks_.begin(),
     event_callbacks_.end(),
-    HandleCompare<ParameterEventCallbackHandle>(handle));
+    HandleCompare<ParameterEventCallbackHandle>(callback_handle.get()));
   if (it != event_callbacks_.end()) {
     event_callbacks_.erase(it);
   } else {
@@ -91,9 +91,10 @@ ParameterEventHandler::add_parameter_callback(
 
 void
 ParameterEventHandler::remove_parameter_callback(
-  const ParameterCallbackHandle * const handle)
+  ParameterCallbackHandle::SharedPtr callback_handle)
 {
   std::lock_guard<std::recursive_mutex> lock(mutex_);
+  auto handle = callback_handle.get();
   auto & container = parameter_callbacks_[{handle->parameter_name, handle->node_name}];
   auto it = std::find_if(
     container.begin(),

rclcpp/test/rclcpp/test_parameter_event_handler.cpp

@@ -29,6 +29,11 @@ class TestParameterEventHandler : public rclcpp::ParameterEventHandler
   {
     event_callback(event);
   }
+
+  size_t num_installed_handlers()
+  {
+    return event_callbacks_.size();
+  }
 };
 
 class TestNode : public ::testing::Test
@@ -282,7 +287,7 @@ TEST_F(TestNode, EventCallback)
       }
     };
 
-  auto event_handle = param_handler->add_parameter_event_callback(cb);
+  auto event_handle1 = param_handler->add_parameter_event_callback(cb);
   auto event_handle2 = param_handler->add_parameter_event_callback(cb2);
 
   bool_param = false;
@@ -297,15 +302,15 @@ TEST_F(TestNode, EventCallback)
   // Test removal of event callback
   received = false;
   bool_param = false;
-  param_handler->remove_parameter_event_callback(event_handle.get());
+  param_handler->remove_parameter_event_callback(event_handle1);
   param_handler->test_event(multiple);
   param_handler->test_event(diff_ns_bool);
   EXPECT_EQ(received, false);
   EXPECT_EQ(bool_param, true);
 
   // Should throw if callback handle no longer exists or already removed
   EXPECT_THROW(
-    param_handler->remove_parameter_event_callback(event_handle.get()), std::runtime_error);
+    param_handler->remove_parameter_event_callback(event_handle1), std::runtime_error);
 }
 
 TEST_F(TestNode, MultipleParameterCallbacks)
@@ -329,18 +334,24 @@ TEST_F(TestNode, MultipleParameterCallbacks)
   // Test removal of parameter callback by callback handle
   received_1 = false;
   received_2 = false;
-  param_handler->remove_parameter_callback(h1.get());
+  param_handler->remove_parameter_callback(h1);
   param_handler->test_event(same_node_int);
   EXPECT_EQ(received_1, false);
   EXPECT_EQ(received_2, true);
 
   // Test removal of parameter callback by name
   received_2 = false;
-  param_handler->remove_parameter_callback(h2.get());
+  param_handler->remove_parameter_callback(h2);
   param_handler->test_event(same_node_int);
   EXPECT_EQ(received_2, false);
 
   // Should throw if callback handle no longer exists or already removed
-  EXPECT_THROW(param_handler->remove_parameter_callback(h1.get()), std::runtime_error);
-  EXPECT_THROW(param_handler->remove_parameter_callback(h2.get()), std::runtime_error);
+  EXPECT_THROW(param_handler->remove_parameter_callback(h1), std::runtime_error);
+  EXPECT_THROW(param_handler->remove_parameter_callback(h2), std::runtime_error);
+
+  param_handler->remove_parameter_callback(h3);
+
+  // All callbacks should have been removed
+  EXPECT_EQ(received_2, 0);
+  EXPECT_EQ(param_handler->num_installed_handlers(), 0UL);
 }