Rename message_bounds to include typesupport

[mjcarroll]

Based on feedback from API review, addresses inconsistency in naming

Closes #467

Signed-off-by: Michael Carroll [email protected]

[mjcarroll]

Test up to rclcpp

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

One minor thing to fix by adding documentation. Otherwise looks good to me and fits the pattern for the other "type support" structures.

[clalancette]

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 :).

[ahcorde]

I started here to document these headers and structs #472 . @mjcarroll , it would be greate if you can review it .

[mjcarroll]

I'll review, thanks for that @ahcorde

[dirk-thomas]

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?

[mjcarroll]

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.

[dirk-thomas]

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?

[jacobperron]

I don't see what was wrong with the original name. From the review ticket:

The naming diverges from all other symbols in this package in terms of not having a common prefix.

So, shouldn't the name have the prefix rosidl_runtime_c_?

I think the name rosidl_runtime_c_message_bounds_t would be fine.

[jacobperron]

On closer look, it looks like the structs in rosidl_runtime_c have two different naming patterns:

rosidl_runtime_c__<NAME>

or

rosidl_<NAME>_type_support_t

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 🤷‍♂️

[jacobperron]

Can we typedef the old name with a deprecation?

[dirk-thomas]

shouldn't the name have the prefix rosidl_runtime_c_?

👍

I think the name rosidl_runtime_c_message_bounds_t would be fine.

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 _t suffix.

So the offline discussed proposed name is: rosidl_runtime_c__Sequence__bound.

[mjcarroll]

• Linux
• Linux-aarch64
• macOS
• Windows

[mjcarroll]

Missed pushing rmw_connext, trying with just Linux to save some CI cycles:

• Linux

[clalancette]

Approved.

[rotu]

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.