-
Notifications
You must be signed in to change notification settings - Fork 481
Add static assert for removed subscription callback signatures #2975
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: mini-1235 <[email protected]>
|
I have pulled and built the ROS2 repos from https://raw.githubusercontent.com/ros2/ros2/rolling/ros2.repos (Please let me know if there are any other repos I should include) Removing
Removing/Deprecating
|
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mini-1235 thanks for fixing up all the related packages and repositories.
changing signatures are totally fine things to do including this PR, but i want to discuss with you on either we should enforcement or deprecation here for the next release.
| static_assert(!is_invalid_signature, | ||
| "std::shared_ptr<> callback signature is unsupported " | ||
| "Use std::shared_ptr<const> or std::unique_ptr<> instead."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that those signatures have been deprecated for a long time as comment states, but my concern is that can we really enforce this all the sudden? i understand this is right thing to do, but this can possibly break the user space without any notification.
IMO, Deprecation Period should still be given for user space for next release, and then we can enforce the invalid signature assertion?
@mini-1235 @mjcarroll @ahcorde what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two points to discuss here:
-
std::shared_ptr<MessageT>: this was deprecated back in August 2021, but the deprecation warning was accidentally removed in February 2024. It looks like the majority of the ROS 2 core repositories are fine, but I am not entirely sure about other packages. For example, Nav2 still usesstd::shared_ptr<MessageT>a lot: mini-1235/navigation2@9a6aca5 -
std::shared_ptr<TypeAdapter/Serialize Message>: this signature was actually never deprecated, so I think it should be deprecated instead of removed
In my current PR, I removed all forms of std::shared_ptr<> mainly for testing purposes. I am open to any suggestions on how this should be handled :)
Description
Fixes #2972
Is this user-facing behavior change?
Yes
Did you use Generative AI?
No
Additional Information
When working on this PR, I noticed that #1713 only deprecates
MessageT. However, looking at the comment here:rclcpp/rclcpp/include/rclcpp/any_subscription_callback.hpp
Lines 140 to 154 in eb49444
it looks like the type_adaptation/serialized message are not yet deprecated, how should we proceed?
In the meantime, I have also updated the tests for type_adaptation/serialized to use
std::shared_ptr<const>