From c1b80bd3673d2683776434d02a0496833ae03508 Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Mon, 27 Apr 2020 22:23:43 -0700 Subject: [PATCH] Serialized message move constructor (#1097) * correct use of move semantics Signed-off-by: Knese Karsten * more tests Signed-off-by: Knese Karsten * make error message more exact Signed-off-by: Karsten Knese * use std::exchange Signed-off-by: Karsten Knese * fix typo Signed-off-by: Karsten Knese --- rclcpp/src/rclcpp/serialization.cpp | 7 ++-- rclcpp/src/rclcpp/serialized_message.cpp | 43 ++++++++++++------------ rclcpp/test/test_serialized_message.cpp | 37 +++++++++++++++++--- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/rclcpp/src/rclcpp/serialization.cpp b/rclcpp/src/rclcpp/serialization.cpp index 689757c..bca3411 100644 --- a/rclcpp/src/rclcpp/serialization.cpp +++ b/rclcpp/src/rclcpp/serialization.cpp @@ -55,8 +55,11 @@ void SerializationBase::deserialize_message( rcpputils::check_true(nullptr != type_support_, "Typesupport is nullpointer."); rcpputils::check_true(nullptr != serialized_message, "Serialized message is nullpointer."); rcpputils::check_true( - 0 != serialized_message->capacity() && 0 != serialized_message->size(), - "Serialized message is wrongly initialized."); + 0u != serialized_message->capacity(), + "Wrongly initialized. Serialized message has a capacity of zero."); + rcpputils::check_true( + 0u != serialized_message->size(), + "Wrongly initialized. Serialized message has a size of zero."); rcpputils::check_true(nullptr != ros_message, "ROS message is a nullpointer."); const auto ret = rmw_deserialize( diff --git a/rclcpp/src/rclcpp/serialized_message.cpp b/rclcpp/src/rclcpp/serialized_message.cpp index 64e54f5..2caaf90 100644 --- a/rclcpp/src/rclcpp/serialized_message.cpp +++ b/rclcpp/src/rclcpp/serialized_message.cpp @@ -66,50 +66,51 @@ SerializedMessage::SerializedMessage(const rcl_serialized_message_t & other) } SerializedMessage::SerializedMessage(SerializedMessage && other) -: SerializedMessage(other.serialized_message_) -{ - other.serialized_message_ = rmw_get_zero_initialized_serialized_message(); -} +: serialized_message_( + std::exchange(other.serialized_message_, rmw_get_zero_initialized_serialized_message())) +{} SerializedMessage::SerializedMessage(rcl_serialized_message_t && other) -: serialized_message_(other) -{ - // reset buffer to prevent double free - other = rmw_get_zero_initialized_serialized_message(); -} +: serialized_message_( + std::exchange(other, rmw_get_zero_initialized_serialized_message())) +{} SerializedMessage & SerializedMessage::operator=(const SerializedMessage & other) { - serialized_message_ = rmw_get_zero_initialized_serialized_message(); - copy_rcl_message(other.serialized_message_, serialized_message_); + if (this != &other) { + serialized_message_ = rmw_get_zero_initialized_serialized_message(); + copy_rcl_message(other.serialized_message_, serialized_message_); + } return *this; } SerializedMessage & SerializedMessage::operator=(const rcl_serialized_message_t & other) { - serialized_message_ = rmw_get_zero_initialized_serialized_message(); - copy_rcl_message(other, serialized_message_); + if (&serialized_message_ != &other) { + serialized_message_ = rmw_get_zero_initialized_serialized_message(); + copy_rcl_message(other, serialized_message_); + } return *this; } SerializedMessage & SerializedMessage::operator=(SerializedMessage && other) { - *this = other.serialized_message_; - other.serialized_message_ = rmw_get_zero_initialized_serialized_message(); + if (this != &other) { + serialized_message_ = + std::exchange(other.serialized_message_, rmw_get_zero_initialized_serialized_message()); + } return *this; } SerializedMessage & SerializedMessage::operator=(rcl_serialized_message_t && other) { - serialized_message_ = rmw_get_zero_initialized_serialized_message(); - serialized_message_ = other; - - // reset original to prevent double free - other = rmw_get_zero_initialized_serialized_message(); - + if (&serialized_message_ != &other) { + serialized_message_ = + std::exchange(other, rmw_get_zero_initialized_serialized_message()); + } return *this; } diff --git a/rclcpp/test/test_serialized_message.cpp b/rclcpp/test/test_serialized_message.cpp index 9047e22..4e28106 100644 --- a/rclcpp/test/test_serialized_message.cpp +++ b/rclcpp/test/test_serialized_message.cpp @@ -52,6 +52,7 @@ TEST(TestSerializedMessage, various_constructors) { rcl_handle.buffer_length = content_size; EXPECT_STREQ(content.c_str(), reinterpret_cast(rcl_handle.buffer)); EXPECT_EQ(content_size, serialized_message.capacity()); + EXPECT_EQ(content_size, serialized_message.size()); // Copy Constructor rclcpp::SerializedMessage other_serialized_message(serialized_message); @@ -158,9 +159,9 @@ TEST(TestSerializedMessage, serialization) { } } -template -void test_empty_msg_serialize() +void serialize_default_ros_msg() { + using MessageT = test_msgs::msg::BasicTypes; rclcpp::Serialization serializer; MessageT ros_msg; rclcpp::SerializedMessage serialized_msg; @@ -168,12 +169,40 @@ void test_empty_msg_serialize() serializer.serialize_message(&ros_msg, &serialized_msg); } -template -void test_empty_msg_deserialize() +void serialize_default_ros_msg_into_nullptr() { + using MessageT = test_msgs::msg::BasicTypes; + rclcpp::Serialization serializer; + MessageT ros_msg; + + serializer.serialize_message(&ros_msg, nullptr); +} + +void deserialize_default_serialized_message() +{ + using MessageT = test_msgs::msg::BasicTypes; rclcpp::Serialization serializer; MessageT ros_msg; rclcpp::SerializedMessage serialized_msg; serializer.deserialize_message(&serialized_msg, &ros_msg); } + +void deserialize_nullptr() +{ + using MessageT = test_msgs::msg::BasicTypes; + rclcpp::Serialization serializer; + MessageT ros_msg; + rclcpp::SerializedMessage serialized_msg; + + serializer.deserialize_message(&serialized_msg, &ros_msg); +} + +TEST(TestSerializedMessage, serialization_empty_messages) +{ + EXPECT_NO_THROW(serialize_default_ros_msg()); + EXPECT_THROW(serialize_default_ros_msg_into_nullptr(), rcpputils::IllegalStateException); + EXPECT_THROW(serialize_default_ros_msg_into_nullptr(), rcpputils::IllegalStateException); + EXPECT_THROW(deserialize_default_serialized_message(), rcpputils::IllegalStateException); + EXPECT_THROW(deserialize_nullptr(), rcpputils::IllegalStateException); +}