Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscription callback should support const reference by default #281

Closed
geoffviola opened this issue Nov 29, 2016 · 4 comments
Closed

Subscription callback should support const reference by default #281

geoffviola opened this issue Nov 29, 2016 · 4 comments
Labels
enhancement New feature or request

Comments

@geoffviola
Copy link

I noticed that the any_subscription_callback supports shared_ptr and unique_ptr, but not const&: https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/any_subscription_callback.hpp. It seems simpler to have const& on the subscription callback. The user almost always uses the data in the callback scope. Generally, he will not change the owner of the message data to avoid copying. The rclpcc example should use it: https://github.com/ros2/examples/blob/master/rclcpp_examples/src/topics/listener.cpp.

@wjwwood
Copy link
Member

wjwwood commented Jan 28, 2017

Yeah, I think it makes sense to support const reference, but never got around to doing. That would be a good enhancement. As for whether or not to use it by default, I think that might also be the right thing to do, but we were trying to stay as close as possible to ROS 1's simple examples (not a reason to not change, but just a historical reason why it is that way right now):

http://wiki.ros.org/ROS/Tutorials/WritingPublisherSubscriber%28c%2B%2B%29#roscpp_tutorials.2BAC8-Tutorials.2BAC8-WritingPublisherSubscriber.CA-b00794bcdcacff4e742e844261078a50000d0a69_34

@henningkayser
Copy link

@wjwwood any updates on this issue? It seems like it would be really easy to add const-ref callbacks and I think it's a very reasonable default. With ROS1 const-ref callbacks were also available and we use them a lot in MoveIt.

@wjwwood
Copy link
Member

wjwwood commented Jun 13, 2020

Nope, no one has had time. It would be a good enhancement though. We need to audit the allowed signatures as well. We did this at some point for publish and deprecated several different styles and added some others.

@wjwwood
Copy link
Member

wjwwood commented Jul 16, 2021

Closed by #1557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants