Fixed clang issues #55
Conversation
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
| @@ -264,25 +264,25 @@ class PointCloudTransport : public PointCloudTransportLoader | |||
| const std::string & base_topic, rmw_qos_profile_t custom_qos, | |||
| void (T::* fp)(const sensor_msgs::msg::PointCloud2::ConstSharedPtr &) const, T * obj, | |||
| const point_cloud_transport::TransportHints * transport_hints = nullptr, | |||
| bool allow_concurrent_callbacks = false) | |||
| bool /*allow_concurrent_callbacks*/ = false) | |||
There was a problem hiding this comment.
I'm going to suggest that instead of doing this, we actually just remove this parameter completely. As far as I can tell, it is never used by any callers or callees.
| } | ||
|
|
||
| template<class T> | ||
| point_cloud_transport::Subscriber subscribe( | ||
| const std::string & base_topic, uint32_t queue_size, | ||
| void (T::* fp)(const sensor_msgs::msg::PointCloud2::ConstSharedPtr &) const, T * obj, | ||
| const point_cloud_transport::TransportHints * transport_hints = nullptr, | ||
| bool allow_concurrent_callbacks = false) | ||
| bool /*allow_concurrent_callbacks*/ = false) |
| @@ -92,7 +94,7 @@ class PublisherPlugin | |||
|
|
|||
| //! Publish a point cloud using the transport associated with this PublisherPlugin. | |||
| POINT_CLOUD_TRANSPORT_PUBLIC | |||
| virtual void publish(const sensor_msgs::msg::PointCloud2::ConstSharedPtr & message) const; | |||
| virtual void publishPtr(const sensor_msgs::msg::PointCloud2::ConstSharedPtr & message) const; | |||
There was a problem hiding this comment.
Out of curiosity, why rename this?
There was a problem hiding this comment.
Two main reasons:
- I was getting this warning
hidden overloaded virtual function 'point_cloud_transport::PublisherPlugin::publish' declared here: different number of parameters (1 vs 2)- To have the same API as image_Transport
| // TODO(ahcorde): Review this, infinite recursion | ||
| // return this->encode(raw); |
There was a problem hiding this comment.
This is concerning, we should probably understand what is happening here.
There was a problem hiding this comment.
I'm getting this error message:
warning: all paths through this function will call itself [-Winfinite-recursion]Signed-off-by: Alejandro Hernández Cordero <[email protected]>
There was a problem hiding this comment.
This looks good to me now, with green CI.
I'll suggest merging in #57 first, then rebasing and running CI on this one (including Clang CI).
| PublisherPlugin::EncodeResult PublisherPlugin::encode(const sensor_msgs::msg::PointCloud2 & raw) | ||
| const | ||
| { | ||
| return this->encode(raw); | ||
| } | ||
|
|
There was a problem hiding this comment.
Oh, I see. Yeah, clang is totally right about the old code; any call to this function will just have it call itself. What we really wanted is to have this as a pure virtual function, which this change now does. So I think this is good now.
|
According with this log https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/1797/console there are some issue with clang. This PR try to fix them