Drop the generic version of get_rcl_allocator.

This allows us to simplify a bunch of code as well.

Signed-off-by: Steve Wolter <[email protected]>

Author: stevewolter

rclcpp/include/rclcpp/allocator/allocator_common.hpp

@@ -29,64 +29,16 @@ namespace allocator
 template<typename T, typename Alloc>
 using AllocRebind = typename std::allocator_traits<Alloc>::template rebind_traits<T>;
 
-template<typename Alloc>
-void * retyped_allocate(size_t size, void * untyped_allocator)
-{
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
-}
-
-template<typename Alloc>
-void retyped_deallocate(void * untyped_pointer, void * untyped_allocator)
-{
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  auto typed_ptr = static_cast<typename Alloc::value_type *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-}
-
-template<typename Alloc>
-void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator)
-{
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  auto typed_ptr = static_cast<typename Alloc::value_type *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-  return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
-}
-
 }  // namespace allocator
 
-// Deprecated: Generic converter from C++ allocator into RCL allocator.
-// This allocator overallocates memory by 100x and invokes UB on free,
-// see #1254.
-template<typename Alloc>
-rcl_allocator_t get_rcl_allocator(Alloc & allocator)
-{
-  rcl_allocator_t rcl_allocator = rcl_get_default_allocator();
-#ifndef _WIN32
-  rcl_allocator.allocate = &allocator::retyped_allocate<Alloc>;
-  rcl_allocator.deallocate = &allocator::retyped_deallocate<Alloc>;
-  rcl_allocator.reallocate = &allocator::retyped_reallocate<Alloc>;
-  rcl_allocator.state = &allocator;
-#else
-  (void)allocator;  // Remove warning
-#endif
-  return rcl_allocator;
-}
-
 // Builds the RCL default allocator for the C++ standard allocator.
 // This assumes that the user intent behind both allocators is the
 // same: Using system malloc for allocation.
+//
+// If you're using a custom allocator in ROS, you'll need to provide
+// your own overload for this function.
 template<typename T>
-rcl_allocator_t get_rcl_allocator(std::allocator<T> & allocator)
+rcl_allocator_t get_rcl_allocator(std::allocator<T> allocator)
 {
   (void)allocator;  // Remove warning
   return rcl_get_default_allocator();

rclcpp/include/rclcpp/publisher_options.hpp

@@ -56,16 +56,14 @@ struct PublisherOptionsBase
 template<typename Allocator>
 struct PublisherOptionsWithAllocator : public PublisherOptionsBase
 {
-  PublisherOptionsWithAllocator<Allocator>()
-  : allocator_(new Allocator()),
-    message_allocator_(*allocator_)
-  {}
+  /// Optional custom allocator.
+  std::shared_ptr<Allocator> allocator = nullptr;
+
+  PublisherOptionsWithAllocator<Allocator>() {}
 
   /// Constructor using base class as input.
   explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base)
-  : PublisherOptionsBase(publisher_options_base),
-    allocator_(new Allocator()),
-    message_allocator_(*allocator_)
+  : PublisherOptionsBase(publisher_options_base)
   {}
 
   /// Convert this class, and a rclcpp::QoS, into an rcl_publisher_options_t.
@@ -74,7 +72,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
   to_rcl_publisher_options(const rclcpp::QoS & qos) const
   {
     rcl_publisher_options_t result = rcl_publisher_get_default_options();
-    result.allocator = get_rcl_allocator(message_allocator_);
+    result.allocator = get_rcl_allocator(*this->get_allocator());
     result.qos = qos.get_rmw_qos_profile();
 
     // Apply payload to rcl_publisher_options if necessary.
@@ -86,19 +84,20 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
   }
 
 
-  /// Get the allocator
+  /// Get the allocator, creating one if needed.
   std::shared_ptr<Allocator>
   get_allocator() const
   {
-    return allocator_;
+    if (!this->allocator) {
+      // TODO(wjwwood): I would like to use the commented line instead, but
+      //   cppcheck 1.89 fails with:
+      //     Syntax Error: AST broken, binary operator '>' doesn't have two operands.
+      // return std::make_shared<Allocator>();
+      std::shared_ptr<Allocator> tmp(new Allocator());
+      return tmp;
+    }
+    return this->allocator;
   }
-
- private:
-  using AllocatorTraits = std::allocator_traits<Allocator>;
-  using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
-
-  std::shared_ptr<Allocator> allocator_;
-  MessageAllocatorT message_allocator_;
 };
 
 using PublisherOptions = PublisherOptionsWithAllocator<std::allocator<void>>;

rclcpp/include/rclcpp/subscription_options.hpp

@@ -78,17 +78,15 @@ struct SubscriptionOptionsBase
 template<typename Allocator>
 struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
 {
-  SubscriptionOptionsWithAllocator<Allocator>()
-  : allocator_(new Allocator()),
-    message_allocator_(*allocator_)
-  {}
+  /// Optional custom allocator.
+  std::shared_ptr<Allocator> allocator = nullptr;
+
+  SubscriptionOptionsWithAllocator<Allocator>() {}
 
   /// Constructor using base class as input.
   explicit SubscriptionOptionsWithAllocator(
     const SubscriptionOptionsBase & subscription_options_base)
-  : SubscriptionOptionsBase(subscription_options_base),
-    allocator_(new Allocator()),
-    message_allocator_(*allocator_)
+  : SubscriptionOptionsBase(subscription_options_base)
   {}
 
   /// Convert this class, with a rclcpp::QoS, into an rcl_subscription_options_t.
@@ -101,7 +99,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
   to_rcl_subscription_options(const rclcpp::QoS & qos) const
   {
     rcl_subscription_options_t result = rcl_subscription_get_default_options();
-    result.allocator = get_rcl_allocator(message_allocator_);
+    result.allocator = get_rcl_allocator(*this->get_allocator());
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_subscription_options.ignore_local_publications = this->ignore_local_publications;
 
@@ -113,19 +111,15 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
     return result;
   }
 
-  /// Get the allocator
+  /// Get the allocator, creating one if needed.
   std::shared_ptr<Allocator>
   get_allocator() const
   {
-    return allocator_
+    if (!this->allocator) {
+      return std::make_shared<Allocator>();
+    }
+    return this->allocator;
   }
-
- private:
-  using AllocatorTraits = std::allocator_traits<Allocator>;
-  using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
-
-  std::shared_ptr<Allocator> allocator_;
-  MessageAllocatorT message_allocator_;
 };
 
 using SubscriptionOptions = SubscriptionOptionsWithAllocator<std::allocator<void>>;

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

@@ -60,7 +60,6 @@ NodeParameters::NodeParameters(
   // TODO(wjwwood): expose this allocator through the Parameter interface.
   rclcpp::PublisherOptionsWithAllocator<AllocatorT> publisher_options(
     parameter_event_publisher_options);
-  publisher_options.allocator = std::make_shared<AllocatorT>();
 
   if (start_parameter_services) {
     parameter_service_ = std::make_shared<ParameterService>(node_base, node_services, this);

rclcpp/test/rclcpp/allocator/test_allocator_common.cpp

@@ -18,33 +18,6 @@
 
 #include "rclcpp/allocator/allocator_common.hpp"
 
-TEST(TestAllocatorCommon, retyped_allocate) {
-  std::allocator<int> allocator;
-  void * untyped_allocator = &allocator;
-  void * allocated_mem =
-    rclcpp::allocator::retyped_allocate<std::allocator<char>>(1u, untyped_allocator);
-  ASSERT_NE(nullptr, allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<std::allocator<int>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
-
-  allocated_mem = allocator.allocate(1);
-  ASSERT_NE(nullptr, allocated_mem);
-  void * reallocated_mem =
-    rclcpp::allocator::retyped_reallocate<std::allocator<int>>(
-    allocated_mem, 2u, untyped_allocator);
-  ASSERT_NE(nullptr, reallocated_mem);
-
-  auto code2 = [&untyped_allocator, reallocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<std::allocator<int>>(
-        reallocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code2());
-}
-
 TEST(TestAllocatorCommon, get_rcl_allocator) {
   std::allocator<int> allocator;
   auto rcl_allocator = rclcpp::get_rcl_allocator(allocator);

rclcpp/test/rclcpp/test_subscription.cpp

@@ -246,7 +246,6 @@ TEST_F(TestSubscription, various_creation_signatures) {
   }
   {
     rclcpp::SubscriptionOptionsWithAllocator<std::allocator<void>> options;
-    options.allocator = std::make_shared<std::allocator<void>>();
     EXPECT_NE(nullptr, options.get_allocator());
     auto sub = rclcpp::create_subscription<Empty>(
       node, "topic", 42, cb, options);