Make loaned messages const on the subscription side#992
Make loaned messages const on the subscription side#992nnmm wants to merge 2 commits intoros2:rollingfrom
Conversation
Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai>
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
I think this makes sense.
|
Can somebody help me understand why the Rpr__rcl__ubuntu_jammy_amd64 check is red? I'm not familiar with Jenkins, and in the console log it says "0 tests failed". |
It's because there are compiler warnings: https://build.ros2.org/job/Rpr__rcl__ubuntu_jammy_amd64/52/gcc/new/ |
Thanks. I'll get on it. |
Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai>
|
Should I also do this change in the rmw layer? |
Yes, and after that it would be nice to cleanup the casts that will become unnecessary here. |
|
|
@nnmm there are some build failures related to the change. |
|
@ivanpauno I'm not sure how this can be tested, but it needs the changes to rclcpp as well. |
ah sorry, my bad. |
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
Sorry, I know this has been sitting a long time. I just took a look and suggested a way to make this better if you are still interested in making this happen.
| bool taken = false; | ||
| rmw_ret_t ret = rmw_take_loaned_message_with_info( | ||
| subscription->impl->rmw_handle, loaned_message, &taken, message_info_local, allocation); | ||
| subscription->impl->rmw_handle, (void **) loaned_message, &taken, message_info_local, |
There was a problem hiding this comment.
It seems to me that rather than doing this cast here, we should first change rmw_take_loaned_message_with_info to take a const void ** loaned_message. At that point, everything will be const and no casts will be needed.
Same goes below for rmw_return_loaned_message_from_subscription.
4 participants
It seems to me that it is an error for the receiver of a loaned message to modify it – other subscriptions could be reading the same message at the same time. Therefore, this PR makes the API stricter by handing out only const pointers.
I have also made corresponding changes in rclcpp, which I'll open a PR for as well and link here.