Make get_rcl_allocator a function family (try 2)

rclcpp/include/rclcpp/allocator/allocator_common.hpp

@@ -30,14 +30,19 @@ 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)
+/// Return the equivalent rcl_allocator_t 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)
 {
-  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);
+  (void)allocator;     // Remove warning
+  return rcl_get_default_allocator();
 }
 
 template<typename Alloc>
@@ -56,61 +61,6 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void *
   return allocated_memory;
 }
 
-template<typename T, 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<T *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-}
-
-template<typename T, 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<T *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-  return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
-}
-
-
-// Convert a std::allocator_traits-formatted Allocator into an rcl allocator
-template<
-  typename T,
-  typename Alloc,
-  typename std::enable_if<!std::is_same<Alloc, std::allocator<void>>::value>::type * = nullptr>
-rcl_allocator_t get_rcl_allocator(Alloc & allocator)
-{
-  rcl_allocator_t rcl_allocator = rcl_get_default_allocator();
-#ifndef _WIN32
-  rcl_allocator.allocate = &retyped_allocate<Alloc>;
-  rcl_allocator.zero_allocate = &retyped_zero_allocate<Alloc>;
-  rcl_allocator.deallocate = &retyped_deallocate<T, Alloc>;
-  rcl_allocator.reallocate = &retyped_reallocate<T, Alloc>;
-  rcl_allocator.state = &allocator;
-#else
-  (void)allocator;  // Remove warning
-#endif
-  return rcl_allocator;
-}
-
-// TODO(jacquelinekay) Workaround for an incomplete implementation of std::allocator<void>
-template<
-  typename T,
-  typename Alloc,
-  typename std::enable_if<std::is_same<Alloc, std::allocator<void>>::value>::type * = nullptr>
-rcl_allocator_t get_rcl_allocator(Alloc & allocator)
-{
-  (void)allocator;
-  return rcl_get_default_allocator();
-}
-
 }  // namespace allocator
 }  // namespace rclcpp
 

rclcpp/include/rclcpp/message_memory_strategy.hpp

@@ -18,6 +18,7 @@
 #include <memory>
 #include <stdexcept>
 
+#include "rcl/allocator.h"
 #include "rcl/types.h"
 
 #include "rclcpp/allocator/allocator_common.hpp"
@@ -61,15 +62,15 @@ class MessageMemoryStrategy
     message_allocator_ = std::make_shared<MessageAlloc>();
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>();
     buffer_allocator_ = std::make_shared<BufferAlloc>();
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    rcutils_allocator_ = rcl_get_default_allocator();
   }
 
   explicit MessageMemoryStrategy(std::shared_ptr<Alloc> allocator)
   {
     message_allocator_ = std::make_shared<MessageAlloc>(*allocator.get());
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>(*allocator.get());
     buffer_allocator_ = std::make_shared<BufferAlloc>(*allocator.get());
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    rcutils_allocator_ = rcl_get_default_allocator();
   }
 
   virtual ~MessageMemoryStrategy() = default;

rclcpp/include/rclcpp/publisher_options.hpp

@@ -20,6 +20,7 @@
 #include <type_traits>
 #include <vector>
 
+#include "rcl/allocator.h"
 #include "rcl/publisher.h"
 
 #include "rclcpp/allocator/allocator_common.hpp"
@@ -77,15 +78,16 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
   /// Constructor using base class as input.
   explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base)
   : PublisherOptionsBase(publisher_options_base)
-  {}
+  {
+  }
 
   /// Convert this class, and a rclcpp::QoS, into an rcl_publisher_options_t.
   template<typename MessageT>
   rcl_publisher_options_t
   to_rcl_publisher_options(const rclcpp::QoS & qos) const
   {
     rcl_publisher_options_t result = rcl_publisher_get_default_options();
-    result.allocator = this->get_rcl_allocator();
+    result.allocator = rcl_get_default_allocator();
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_publisher_options.require_unique_network_flow_endpoints =
       this->require_unique_network_flow_endpoints;
@@ -98,7 +100,6 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
     return result;
   }
 
-
   /// Get the allocator, creating one if needed.
   std::shared_ptr<Allocator>
   get_allocator() const
@@ -123,7 +124,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
       plain_allocator_storage_ =
         std::make_shared<PlainAllocator>(*this->get_allocator());
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+    return rcl_get_default_allocator();
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

@@ -424,7 +424,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy
 
   rcl_allocator_t get_allocator() override
   {
-    return rclcpp::allocator::get_rcl_allocator<void *, VoidAlloc>(*allocator_.get());
+    return rcl_get_default_allocator();
   }
 
   size_t number_of_ready_subscriptions() const override

rclcpp/include/rclcpp/subscription_options.hpp

@@ -21,6 +21,8 @@
 #include <type_traits>
 #include <vector>
 
+#include "rcl/allocator.h"
+
 #include "rclcpp/callback_group.hpp"
 #include "rclcpp/detail/rmw_implementation_specific_subscription_payload.hpp"
 #include "rclcpp/intra_process_buffer_type.hpp"
@@ -103,7 +105,8 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
   explicit SubscriptionOptionsWithAllocator(
     const SubscriptionOptionsBase & subscription_options_base)
   : SubscriptionOptionsBase(subscription_options_base)
-  {}
+  {
+  }
 
   /// Convert this class, with a rclcpp::QoS, into an rcl_subscription_options_t.
   /**
@@ -115,7 +118,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 = this->get_rcl_allocator();
+    result.allocator = rcl_get_default_allocator();
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_subscription_options.ignore_local_publications = this->ignore_local_publications;
     result.rmw_subscription_options.require_unique_network_flow_endpoints =
@@ -167,7 +170,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
       plain_allocator_storage_ =
         std::make_shared<PlainAllocator>(*this->get_allocator());
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+    return rcl_get_default_allocator();
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

rclcpp/test/rclcpp/allocator/test_allocator_common.cpp

@@ -18,100 +18,41 @@
 
 #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);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
-
-  allocated_mem = allocator.allocate(1);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != allocated_mem);
-  void * reallocated_mem =
-    rclcpp::allocator::retyped_reallocate<int, std::allocator<int>>(
-    allocated_mem, 2u, untyped_allocator);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != reallocated_mem);
-
-  auto code2 = [&untyped_allocator, reallocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        reallocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code2());
-}
-
-TEST(TestAllocatorCommon, retyped_zero_allocate_basic) {
+TEST(TestAllocatorCommon, retyped_zero_allocate_basic)
+{
   std::allocator<int> allocator;
   void * untyped_allocator = &allocator;
   void * allocated_mem =
     rclcpp::allocator::retyped_zero_allocate<std::allocator<char>>(20u, 1u, untyped_allocator);
   ASSERT_TRUE(nullptr != allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<char, std::allocator<char>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
 }
 
-TEST(TestAllocatorCommon, retyped_zero_allocate) {
+TEST(TestAllocatorCommon, retyped_zero_allocate)
+{
   std::allocator<int> allocator;
   void * untyped_allocator = &allocator;
   void * allocated_mem =
     rclcpp::allocator::retyped_zero_allocate<std::allocator<char>>(20u, 1u, untyped_allocator);
   // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
   // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
   ASSERT_TRUE(nullptr != allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
-
-  allocated_mem = allocator.allocate(1);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != allocated_mem);
-  void * reallocated_mem =
-    rclcpp::allocator::retyped_reallocate<int, std::allocator<int>>(
-    allocated_mem, 2u, untyped_allocator);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != reallocated_mem);
-
-  auto code2 = [&untyped_allocator, reallocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        reallocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code2());
 }
 
-TEST(TestAllocatorCommon, get_rcl_allocator) {
+TEST(TestAllocatorCommon, get_rcl_allocator)
+{
   std::allocator<int> allocator;
-  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator<int>(allocator);
+  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator);
   EXPECT_NE(nullptr, rcl_allocator.allocate);
   EXPECT_NE(nullptr, rcl_allocator.deallocate);
   EXPECT_NE(nullptr, rcl_allocator.reallocate);
   EXPECT_NE(nullptr, rcl_allocator.zero_allocate);
   // Not testing state as that may or may not be null depending on platform
 }
 
-TEST(TestAllocatorCommon, get_void_rcl_allocator) {
+TEST(TestAllocatorCommon, get_void_rcl_allocator)
+{
   std::allocator<void> allocator;
-  auto rcl_allocator =
-    rclcpp::allocator::get_rcl_allocator<void, std::allocator<void>>(allocator);
+  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator);
   EXPECT_NE(nullptr, rcl_allocator.allocate);
   EXPECT_NE(nullptr, rcl_allocator.deallocate);
   EXPECT_NE(nullptr, rcl_allocator.reallocate);