Rename message_bounds to include typesupport#475
Conversation
|
Test up to
|
clalancette
left a comment
There was a problem hiding this comment.
One minor thing to fix by adding documentation. Otherwise looks good to me and fits the pattern for the other "type support" structures.
| const rosidl_bounds_type_support_t *, const char *); | ||
|
|
||
| struct rosidl_message_bounds_t | ||
| struct rosidl_bounds_type_support_t |
There was a problem hiding this comment.
While we are in here, we may as well document what this structure is meant to do, and what the fields are. I know we are missing it from the other "type support" structures, but you gotta start somewhere :).
There was a problem hiding this comment.
I started here to document these headers and structs #472 . @mjcarroll , it would be greate if you can review it .
|
Maybe I don't understand well enough how this struct is being used. How is it related to type support? Or to ask more generically what is it defining an upper bound for? |
After looking at it, I realized that the implementation remains incomplete (eg ros2/rmw_connext#346 and ros2/rosidl_typesupport_connext#24), but I can summarize the idea. It is intended to allow users to pass optional upper bounds for messages with unbounded fields in order to get estimates of serialized sizes and appropriately preallocate them. The idea is that it would be an alternative to using entirely fixed sized or bounded messages, which aren't widely used in the ROS2 ecosystem. |
That doesn't sound related to "typesupport" then? |
|
I don't see what was wrong with the original name. From the review ticket:
So, shouldn't the name have the prefix I think the name |
|
On closer look, it looks like the structs in
or
The latter seems more consistent to me for the message bounds structure, as it's contents are similar to the others that follow this pattern 🤷♂️ |
|
Can we typedef the old name with a deprecation? |
👍
First, it isn't a boundary for messages but sequences. Second, it is a single bound - not multiple. And other symbols like the bound for strings doesn't have the So the offline discussed proposed name is: |
Based on feedback from API review. Signed-off-by: Michael Carroll <michael@openrobotics.org>
92c4db3 to
d3a5c5e
Compare
|
|
Missed pushing
|
|
Approved. |
|
I think you missed something https://github.com/ros2/rmw_cyclonedds/pull/168/checks?check_run_id=617219521#step:4:5834 I'm sorry CI has been such a mess that it's hard to get actionable intelligence out of. I'm working on improving it. |
Based on feedback from API review, addresses inconsistency in naming
Closes #467
Signed-off-by: Michael Carroll michael@openrobotics.org