Deprecate shared ptr publish (#709)

* Deprecate publish methods with shared_ptr signature

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected with PR comments. Deprecated similar methods in lifecycle publisher

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Removed reference in unique_ptr publish call

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected with PR comments. Corrected warning problem in lifecycle_publisher

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Deprecate publish call taking a raw ptr. Stop deprecating publish methods in LifecyclePublisher.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Pleased linter

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected mac warning

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Corrected serialized publish methods

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Avoid windows warning

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

* Not deprecate on windows

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

rclcpp/include/rclcpp/publisher.hpp

@@ -97,11 +97,10 @@ public:
    * \param[in] msg A shared pointer to the message to send.
    */
   virtual void
-  publish(std::unique_ptr<MessageT, MessageDeleter> & msg)
+  publish(std::unique_ptr<MessageT, MessageDeleter> msg)
   {
     if (!intra_process_is_enabled_) {
       this->do_inter_process_publish(msg.get());
-      msg.reset();
       return;
     }
     // If an interprocess subscription exist, then the unique_ptr is promoted
@@ -128,6 +127,12 @@ public:
     }
   }
 
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+  [[deprecated(
+    "publishing an unique_ptr is prefered when using intra process communication."
+    " If using a shared_ptr, use publish(*msg).")]]
+#endif
   virtual void
   publish(const std::shared_ptr<const MessageT> & msg)
   {
@@ -148,9 +153,14 @@ public:
     auto ptr = MessageAllocTraits::allocate(*message_allocator_.get(), 1);
     MessageAllocTraits::construct(*message_allocator_.get(), ptr, msg);
     MessageUniquePtr unique_msg(ptr, message_deleter_);
-    this->publish(unique_msg);
+    this->publish(std::move(unique_msg));
   }
 
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+  [[deprecated(
+    "Use publish(*msg). Check against nullptr before calling if necessary.")]]
+#endif
   virtual void
   publish(const MessageT * msg)
   {
@@ -161,22 +171,31 @@ public:
   }
 
   void
-  publish(const rcl_serialized_message_t * serialized_msg)
+  publish(const rcl_serialized_message_t & serialized_msg)
   {
-    if (intra_process_is_enabled_) {
+    return this->do_serialized_publish(&serialized_msg);
-      // TODO(Karsten1987): support serialized message passed by intraprocess
-      throw std::runtime_error("storing serialized messages in intra process is not supported yet");
-    }
-    auto status = rcl_publish_serialized_message(&publisher_handle_, serialized_msg, nullptr);
-    if (RCL_RET_OK != status) {
-      rclcpp::exceptions::throw_from_rcl_error(status, "failed to publish serialized message");
-    }
   }
 
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+  [[deprecated(
+    "Use publish(*serialized_msg). Check against nullptr before calling if necessary.")]]
+#endif
+  void
+  publish(const rcl_serialized_message_t * serialized_msg)
+  {
+    return this->do_serialized_publish(serialized_msg);
+  }
+
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+  [[deprecated(
+    "Use publish(*serialized_msg). Check against nullptr before calling if necessary.")]]
+#endif
   void
   publish(std::shared_ptr<const rcl_serialized_message_t> serialized_msg)
   {
-    return this->publish(serialized_msg.get());
+    return this->do_serialized_publish(serialized_msg.get());
   }
 
   std::shared_ptr<MessageAlloc> get_allocator() const
@@ -204,6 +223,19 @@ protected:
     }
   }
 
+  void
+  do_serialized_publish(const rcl_serialized_message_t * serialized_msg)
+  {
+    if (intra_process_is_enabled_) {
+      // TODO(Karsten1987): support serialized message passed by intraprocess
+      throw std::runtime_error("storing serialized messages in intra process is not supported yet");
+    }
+    auto status = rcl_publish_serialized_message(&publisher_handle_, serialized_msg, nullptr);
+    if (RCL_RET_OK != status) {
+      rclcpp::exceptions::throw_from_rcl_error(status, "failed to publish serialized message");
+    }
+  }
+
   void
   do_intra_process_publish(uint64_t message_seq)
   {

rclcpp/test/test_time_source.cpp

@@ -136,9 +136,9 @@ void trigger_clock_changes(
     if (!rclcpp::ok()) {
       break;  // Break for ctrl-c
     }
-    auto msg = std::make_shared<rosgraph_msgs::msg::Clock>();
+    rosgraph_msgs::msg::Clock msg;
-    msg->clock.sec = i;
+    msg.clock.sec = i;
-    msg->clock.nanosec = 1000;
+    msg.clock.nanosec = 1000;
     clock_pub->publish(msg);
 
     // workaround.  Long-term, there can be a more elegant fix where we hook a future up

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_publisher.hpp

@@ -17,6 +17,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "rclcpp/publisher.hpp"
 
@@ -79,7 +80,7 @@ public:
    * to the actual rclcpp Publisher base class
    */
   virtual void
-  publish(std::unique_ptr<MessageT, MessageDeleter> & msg)
+  publish(std::unique_ptr<MessageT, MessageDeleter> msg)
   {
     if (!enabled_) {
       RCLCPP_WARN(logger_,
@@ -88,26 +89,7 @@ public:
 
       return;
     }
-    rclcpp::Publisher<MessageT, Alloc>::publish(msg);
+    rclcpp::Publisher<MessageT, Alloc>::publish(std::move(msg));
-  }
-
-  /// LifecyclePublisher publish function
-  /**
-   * The publish function checks whether the communication
-   * was enabled or disabled and forwards the message
-   * to the actual rclcpp Publisher base class
-   */
-  virtual void
-  publish(const std::shared_ptr<const MessageT> & msg)
-  {
-    if (!enabled_) {
-      RCLCPP_WARN(logger_,
-        "Trying to publish message on the topic '%s', but the publisher is not activated",
-        this->get_topic_name());
-
-      return;
-    }
-    rclcpp::Publisher<MessageT, Alloc>::publish(*msg);
   }
 
   /// LifecyclePublisher publish function
@@ -129,6 +111,39 @@ public:
     rclcpp::Publisher<MessageT, Alloc>::publish(msg);
   }
 
+  /// LifecyclePublisher publish function
+  /**
+   * The publish function checks whether the communication
+   * was enabled or disabled and forwards the message
+   * to the actual rclcpp Publisher base class
+   */
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+// Avoid raising a deprecated warning in template specialization in linux.
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+  [[deprecated(
+    "publishing an unique_ptr is prefered when using intra process communication."
+    " If using a shared_ptr, use publish(*msg).")]]
+#endif
+  virtual void
+  publish(const std::shared_ptr<const MessageT> & msg)
+  {
+    if (!enabled_) {
+      RCLCPP_WARN(logger_,
+        "Trying to publish message on the topic '%s', but the publisher is not activated",
+        this->get_topic_name());
+
+      return;
+    }
+    rclcpp::Publisher<MessageT, Alloc>::publish(*msg);
+  }
+
+// Skip deprecated attribute in windows, as it raise a warning in template specialization.
+#if !defined(_WIN32)
+  [[deprecated(
+    "Use publish(*msg). Check against nullptr before calling if necessary.")]]
+#endif
   virtual void
   publish(const MessageT * msg)
   {
@@ -138,6 +153,10 @@ public:
     this->publish(*msg);
   }
 
+#if !defined(_WIN32)
+# pragma GCC diagnostic pop
+#endif
+
   virtual void
   on_activate()
   {