From e94dac402d2dd605506370bd33bf5271f4d9fe89 Mon Sep 17 00:00:00 2001 From: Knese Karsten Date: Mon, 27 Apr 2020 16:08:38 -0700 Subject: [PATCH 1/5] correct use of move semantics Signed-off-by: Knese Karsten --- rclcpp/src/rclcpp/serialized_message.cpp | 2 +- rclcpp/test/test_serialized_message.cpp | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/serialized_message.cpp b/rclcpp/src/rclcpp/serialized_message.cpp index 64e54f58f9..55338b2aa9 100644 --- a/rclcpp/src/rclcpp/serialized_message.cpp +++ b/rclcpp/src/rclcpp/serialized_message.cpp @@ -66,7 +66,7 @@ SerializedMessage::SerializedMessage(const rcl_serialized_message_t & other) } SerializedMessage::SerializedMessage(SerializedMessage && other) -: SerializedMessage(other.serialized_message_) +: SerializedMessage(static_cast(other.serialized_message_)) { other.serialized_message_ = rmw_get_zero_initialized_serialized_message(); } diff --git a/rclcpp/test/test_serialized_message.cpp b/rclcpp/test/test_serialized_message.cpp index 9047e22b22..1f782993b4 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); From fe790c1f0c6088e8cdcdf220eb5078132e59feaa Mon Sep 17 00:00:00 2001 From: Knese Karsten Date: Mon, 27 Apr 2020 16:34:08 -0700 Subject: [PATCH 2/5] more tests Signed-off-by: Knese Karsten --- rclcpp/test/test_serialized_message.cpp | 36 ++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/rclcpp/test/test_serialized_message.cpp b/rclcpp/test/test_serialized_message.cpp index 1f782993b4..4e28106a88 100644 --- a/rclcpp/test/test_serialized_message.cpp +++ b/rclcpp/test/test_serialized_message.cpp @@ -159,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; @@ -169,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); +} From d5262ff103a0fd7753f41d132f80266b1081f7c2 Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Mon, 27 Apr 2020 18:29:40 -0700 Subject: [PATCH 3/5] make error message more exact Signed-off-by: Karsten Knese --- rclcpp/src/rclcpp/serialization.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/serialization.cpp b/rclcpp/src/rclcpp/serialization.cpp index 689757c98c..d614241dcc 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. Serializaed 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( From 205ab58db922ba530f0e6f096a0c7be8e383a47c Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Mon, 27 Apr 2020 19:36:25 -0700 Subject: [PATCH 4/5] use std::exchange Signed-off-by: Karsten Knese --- rclcpp/src/rclcpp/serialized_message.cpp | 43 ++++++++++++------------ 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/rclcpp/src/rclcpp/serialized_message.cpp b/rclcpp/src/rclcpp/serialized_message.cpp index 55338b2aa9..2caaf90edb 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(static_cast(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; } From 31d3addc245ee98cf6a8e34b834c171adb8d2cf2 Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Mon, 27 Apr 2020 19:42:02 -0700 Subject: [PATCH 5/5] fix typo Signed-off-by: Karsten Knese --- rclcpp/src/rclcpp/serialization.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/serialization.cpp b/rclcpp/src/rclcpp/serialization.cpp index d614241dcc..bca341185f 100644 --- a/rclcpp/src/rclcpp/serialization.cpp +++ b/rclcpp/src/rclcpp/serialization.cpp @@ -56,7 +56,7 @@ void SerializationBase::deserialize_message( rcpputils::check_true(nullptr != serialized_message, "Serialized message is nullpointer."); rcpputils::check_true( 0u != serialized_message->capacity(), - "Wrongly initialized. Serializaed message has a capacity of zero."); + "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.");