Refactor serialization support to use allocators and refs

Signed-off-by: methylDragon <[email protected]>

Author: methylDragon

rclcpp/include/rclcpp/dynamic_typesupport/dynamic_message_type_support.hpp

@@ -37,19 +37,18 @@ namespace rclcpp
 namespace dynamic_typesupport
 {
 
-/// Utility wrapper class for `rosidl_message_type_support_t *` containing managed
+/// Utility wrapper class for `rosidl_message_type_support_t` containing managed
 /// instances of the typesupport handle impl.
 /**
  *
  * NOTE: This class is the recommended way to obtain the dynamic message type
- *       support struct, instead of `rcl_dynamic_message_type_support_handle_create()`,
+ *       support struct, instead of `rcl_dynamic_message_type_support_handle_init()`,
  *       because this class will manage the lifetimes for you.
  *
  *       Do NOT call rcl_dynamic_message_type_support_handle_fini!!
  *
  * This class:
- * - Manages the lifetime of the raw pointer.
- * - Exposes getter methods to get the raw pointer and shared pointers
+ * - Exposes getter methods for the struct
  * - Stores shared pointers to wrapper classes that expose the underlying
  *   serialization support API
  *

rclcpp/include/rclcpp/dynamic_typesupport/dynamic_serialization_support.hpp

@@ -15,6 +15,8 @@
 #ifndef RCLCPP__DYNAMIC_TYPESUPPORT__DYNAMIC_SERIALIZATION_SUPPORT_HPP_
 #define RCLCPP__DYNAMIC_TYPESUPPORT__DYNAMIC_SERIALIZATION_SUPPORT_HPP_
 
+#include <rcl/allocator.h>
+#include <rosidl_dynamic_typesupport/api/serialization_support.h>
 #include <rosidl_dynamic_typesupport/types.h>
 
 #include <memory>
@@ -47,21 +49,18 @@ class DynamicSerializationSupport : public std::enable_shared_from_this<DynamicS
 
   // CONSTRUCTION ==================================================================================
   RCLCPP_PUBLIC
-  DynamicSerializationSupport();
+  explicit DynamicSerializationSupport(rcl_allocator_t allocator = rcl_get_default_allocator());
 
   /// Get the rmw middleware implementation specific serialization support (configured by name)
   RCLCPP_PUBLIC
-  explicit DynamicSerializationSupport(const std::string & serialization_library_name);
+  DynamicSerializationSupport(
+    const std::string & serialization_library_name,
+    rcl_allocator_t allocator = rcl_get_default_allocator());
 
-  /// Assume ownership of raw pointer
+  /// Assume ownership of reference
   RCLCPP_PUBLIC
   explicit DynamicSerializationSupport(
-    rosidl_dynamic_typesupport_serialization_support_t * rosidl_serialization_support);
-
-  /// Copy shared pointer
-  RCLCPP_PUBLIC
-  DynamicSerializationSupport(
-    std::shared_ptr<rosidl_dynamic_typesupport_serialization_support_t> serialization_support);
+    rosidl_dynamic_typesupport_serialization_support_t && rosidl_serialization_support);
 
   /// Move constructor
   RCLCPP_PUBLIC
@@ -81,25 +80,18 @@ class DynamicSerializationSupport : public std::enable_shared_from_this<DynamicS
   get_serialization_library_identifier() const;
 
   RCLCPP_PUBLIC
-  rosidl_dynamic_typesupport_serialization_support_t *
+  rosidl_dynamic_typesupport_serialization_support_t &
   get_rosidl_serialization_support();
 
   RCLCPP_PUBLIC
-  const rosidl_dynamic_typesupport_serialization_support_t *
+  const rosidl_dynamic_typesupport_serialization_support_t &
   get_rosidl_serialization_support() const;
 
-  RCLCPP_PUBLIC
-  std::shared_ptr<rosidl_dynamic_typesupport_serialization_support_t>
-  get_shared_rosidl_serialization_support();
-
-  RCLCPP_PUBLIC
-  std::shared_ptr<const rosidl_dynamic_typesupport_serialization_support_t>
-  get_shared_rosidl_serialization_support() const;
-
 protected:
   RCLCPP_DISABLE_COPY(DynamicSerializationSupport)
 
-  std::shared_ptr<rosidl_dynamic_typesupport_serialization_support_t> rosidl_serialization_support_;
+private:
+  rosidl_dynamic_typesupport_serialization_support_t rosidl_serialization_support_;
 };
 
 

rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message.cpp

@@ -14,6 +14,7 @@
 
 #include <rosidl_dynamic_typesupport/api/dynamic_type.h>
 #include <rosidl_dynamic_typesupport/api/dynamic_data.h>
+#include <rosidl_dynamic_typesupport/api/serialization_support.h>
 #include <rosidl_dynamic_typesupport/types.h>
 
 #include <memory>
@@ -248,20 +249,12 @@ DynamicMessage::match_serialization_support_(
   bool out = true;
 
   if (serialization_support.get_serialization_library_identifier() != std::string(
-      rosidl_dynamic_type_data.serialization_support->library_identifier))
+      rosidl_dynamic_type_data.serialization_support->serialization_library_identifier))
   {
     RCUTILS_LOG_ERROR("serialization support library identifier does not match dynamic data's");
     out = false;
   }
 
-  // TODO(methylDragon): Can I do this?? Is it portable?
-  if (serialization_support.get_rosidl_serialization_support() !=
-    rosidl_dynamic_type_data.serialization_support)
-  {
-    RCUTILS_LOG_ERROR("serialization support pointer does not match dynamic data's");
-    out = false;
-  }
-
   return out;
 }
 
@@ -270,7 +263,7 @@ DynamicMessage::match_serialization_support_(
 const std::string
 DynamicMessage::get_serialization_library_identifier() const
 {
-  return std::string(rosidl_dynamic_data_->serialization_support->library_identifier);
+  return std::string(rosidl_dynamic_data_->serialization_support->serialization_library_identifier);
 }
 
 

rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type.cpp

@@ -14,6 +14,7 @@
 
 #include <rosidl_dynamic_typesupport/api/dynamic_data.h>
 #include <rosidl_dynamic_typesupport/api/dynamic_type.h>
+#include <rosidl_dynamic_typesupport/api/serialization_support.h>
 #include <rosidl_dynamic_typesupport/types.h>
 
 #include <memory>
@@ -160,7 +161,9 @@ DynamicMessageType::init_from_description(
 
   rosidl_dynamic_typesupport_dynamic_type_t * rosidl_dynamic_type = nullptr;
   rcutils_ret_t ret = rosidl_dynamic_typesupport_dynamic_type_create_from_description(
-    serialization_support_->get_rosidl_serialization_support(), &description, &rosidl_dynamic_type);
+    &serialization_support_->get_rosidl_serialization_support(),
+    &description,
+    &rosidl_dynamic_type);
   if (ret != RCUTILS_RET_OK || !rosidl_dynamic_type) {
     throw std::runtime_error("could not create new dynamic type object");
   }
@@ -182,19 +185,12 @@ DynamicMessageType::match_serialization_support_(
   bool out = true;
 
   if (serialization_support.get_serialization_library_identifier() != std::string(
-      rosidl_dynamic_type.serialization_support->library_identifier))
+      rosidl_dynamic_type.serialization_support->serialization_library_identifier))
   {
     RCUTILS_LOG_ERROR(
-      "serialization support library identifier does not match dynamic type's");
-    out = false;
-  }
-
-  // TODO(methylDragon): Can I do this?? Is it portable?
-  if (serialization_support.get_rosidl_serialization_support() !=
-    rosidl_dynamic_type.serialization_support)
-  {
-    RCUTILS_LOG_ERROR(
-      "serialization support pointer does not match dynamic type's");
+      "serialization support library identifier does not match dynamic type's (%s vs %s)",
+      serialization_support.get_serialization_library_identifier().c_str(),
+      rosidl_dynamic_type.serialization_support->serialization_library_identifier);
     out = false;
   }
 
@@ -206,7 +202,7 @@ DynamicMessageType::match_serialization_support_(
 const std::string
 DynamicMessageType::get_serialization_library_identifier() const
 {
-  return std::string(rosidl_dynamic_type_->serialization_support->library_identifier);
+  return std::string(rosidl_dynamic_type_->serialization_support->serialization_library_identifier);
 }
 
 

rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_builder.cpp

@@ -14,6 +14,7 @@
 
 #include <rosidl_dynamic_typesupport/api/dynamic_data.h>
 #include <rosidl_dynamic_typesupport/api/dynamic_type.h>
+#include <rosidl_dynamic_typesupport/api/serialization_support.h>
 #include <rosidl_dynamic_typesupport/types.h>
 
 #include <memory>
@@ -154,7 +155,8 @@ DynamicMessageTypeBuilder::init_from_description(
 
   rosidl_dynamic_typesupport_dynamic_type_builder_t * rosidl_dynamic_type_builder = nullptr;
   rcutils_ret_t ret = rosidl_dynamic_typesupport_dynamic_type_builder_create_from_description(
-    serialization_support_->get_rosidl_serialization_support(), &description,
+    &serialization_support_->get_rosidl_serialization_support(),
+    &description,
     &rosidl_dynamic_type_builder);
   if (ret != RCUTILS_RET_OK || !rosidl_dynamic_type_builder) {
     throw std::runtime_error("could not create new dynamic type builder object");
@@ -177,14 +179,14 @@ DynamicMessageTypeBuilder::init_from_serialization_support_(
   if (!serialization_support) {
     throw std::runtime_error("serialization support cannot be nullptr!");
   }
-  if (!serialization_support->get_rosidl_serialization_support()) {
+  if (!&serialization_support->get_rosidl_serialization_support()) {
     throw std::runtime_error("serialization support raw pointer cannot be nullptr!");
   }
 
 
   rosidl_dynamic_typesupport_dynamic_type_builder_t * rosidl_dynamic_type_builder = nullptr;
   rcutils_ret_t ret = rosidl_dynamic_typesupport_dynamic_type_builder_create(
-    serialization_support->get_rosidl_serialization_support(),
+    &serialization_support->get_rosidl_serialization_support(),
     name.c_str(), name.size(),
     &rosidl_dynamic_type_builder);
   if (ret != RCUTILS_RET_OK) {
@@ -212,22 +214,13 @@ DynamicMessageTypeBuilder::match_serialization_support_(
   bool out = true;
 
   if (serialization_support.get_serialization_library_identifier() != std::string(
-      rosidl_dynamic_type_builder.serialization_support->library_identifier))
+      rosidl_dynamic_type_builder.serialization_support->serialization_library_identifier))
   {
     RCUTILS_LOG_ERROR(
       "serialization support library identifier does not match dynamic type builder's");
     out = false;
   }
 
-  // TODO(methylDragon): Can I do this?? Is it portable?
-  if (serialization_support.get_rosidl_serialization_support() !=
-    rosidl_dynamic_type_builder.serialization_support)
-  {
-    RCUTILS_LOG_ERROR(
-      "serialization support pointer does not match dynamic type builder's");
-    out = false;
-  }
-
   return out;
 }
 
@@ -236,7 +229,7 @@ DynamicMessageTypeBuilder::match_serialization_support_(
 const std::string
 DynamicMessageTypeBuilder::get_serialization_library_identifier() const
 {
-  return std::string(rosidl_dynamic_type_builder_->serialization_support->library_identifier);
+  return std::string(rosidl_dynamic_type_builder_->serialization_support->serialization_library_identifier);
 }
 
 

rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_support.cpp

@@ -65,7 +65,7 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport(
   }
   if (ret != RCL_RET_OK) {
     std::string error_msg =
-      std::string("error initializing rosidl message type support:\n") + rcl_get_error_string().str;
+      std::string("error initializing rosidl message type support: ") + rcl_get_error_string().str;
     rcl_reset_error();
     throw std::runtime_error(error_msg);
   }
@@ -75,16 +75,17 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport(
     throw std::runtime_error("rosidl message type support is of the wrong type");
   }
 
-  auto ts_impl = static_cast<const rosidl_dynamic_message_type_support_impl_t *>(
-    rosidl_message_type_support_.data);
+  auto ts_impl = static_cast<rosidl_dynamic_message_type_support_impl_t *>(const_cast<void *>(
+      rosidl_message_type_support_.data));
 
-  serialization_support_ = DynamicSerializationSupport::make_shared(ts_impl->serialization_support);
+  serialization_support_ = DynamicSerializationSupport::make_shared(
+    std::move(ts_impl->serialization_support));
 
   dynamic_message_type_ = DynamicMessageType::make_shared(
-    get_shared_dynamic_serialization_support(), ts_impl->dynamic_message_type);
+    get_shared_dynamic_serialization_support(), std::move(ts_impl->dynamic_message_type));
 
   dynamic_message_ = DynamicMessage::make_shared(
-    get_shared_dynamic_serialization_support(), ts_impl->dynamic_message);
+    get_shared_dynamic_serialization_support(), std::move(ts_impl->dynamic_message));
 }
 
 DynamicMessageTypeSupport::DynamicMessageTypeSupport(
@@ -111,7 +112,7 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport(
   }
 
   rcutils_ret_t ret = rosidl_dynamic_message_type_support_handle_init(
-    serialization_support->get_rosidl_serialization_support(),
+    &serialization_support->get_rosidl_serialization_support(),
     &type_hash,       // type_hash
     &description,     // type_description
     nullptr,          // type_description_sources (not implemented for dynamic types)
@@ -174,40 +175,20 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport(
             "dynamic message library identifier.");
   }
 
-  // Check pointers
-  /* *INDENT-OFF* */
-  if (serialization_support->get_rosidl_serialization_support() !=
-      dynamic_message_type
-        ->get_shared_dynamic_serialization_support()
-        ->get_rosidl_serialization_support())
-  {
-    throw std::runtime_error("serialization support pointer does not match dynamic message type's");
-  }
-  if (dynamic_message_type
-        ->get_shared_dynamic_serialization_support()
-        ->get_rosidl_serialization_support() !=
-      dynamic_message_type
-        ->get_shared_dynamic_serialization_support()
-        ->get_rosidl_serialization_support())
-  {
-    throw std::runtime_error("serialization support does not match pointer dynamic message type's");
-  }
-  /* *INDENT-ON* */
-
   rosidl_type_hash_t type_hash;
   rcutils_ret_t hash_ret = rcl_calculate_type_hash(
     // TODO(methylDragon): Swap this out with the conversion function when it is ready
     //   from https://github.com/ros2/rcl/pull/1052
     reinterpret_cast<const type_description_interfaces__msg__TypeDescription *>(&description),
     &type_hash);
   if (hash_ret != RCL_RET_OK) {
-    std::string error_msg = std::string("failed to get type hash:\n") + rcl_get_error_string().str;
+    std::string error_msg = std::string("failed to get type hash: ") + rcl_get_error_string().str;
     rcl_reset_error();
     throw std::runtime_error(error_msg);
   }
 
   auto ts_impl = static_cast<rosidl_dynamic_message_type_support_impl_t *>(
-    allocator.zero_allocate(1, sizeof(rosidl_dynamic_message_type_support_impl_t), &allocator.state)
+    allocator.zero_allocate(1, sizeof(rosidl_dynamic_message_type_support_impl_t), allocator.state)
   );
   if (!ts_impl) {
     throw std::runtime_error("could not allocate rosidl_message_type_support_t");
@@ -255,7 +236,7 @@ DynamicMessageTypeSupport::~DynamicMessageTypeSupport()
   rosidl_runtime_c__type_description__TypeDescription__fini(&ts_impl->type_description);
   rosidl_runtime_c__type_description__TypeSource__Sequence__fini(
     &ts_impl->type_description_sources);
-  allocator.deallocate(static_cast<void *>(ts_impl), &allocator.state);  // Always C allocated
+  allocator.deallocate(static_cast<void *>(ts_impl), allocator.state);  // Always C allocated
 }