[rosidl_typesupport_introspection_c] Fix get_function and get_const_function semantics for arrays

[ivanpauno]

Fixes #522.
For all arrays/sequences/bounded_sequences in rosidl_typesupport_introspection_c/cpp, the following code works:

member->get_function(ros_message + member->offset_, index)

except for rosidl_typesupport_introspection_c arrays, where the correct code is:

member->get_function(&(ros_message + member->offset_), index)

This PR fixes the semantic difference.

The following PRs must be merged simultaneously:

• Fix array get_function semantics rmw_cyclonedds#248
• Fix array get_function semantics rmw_fastrtps#448
• Document API break in rosidl_typesupport_introspection_c ros2_documentation#877.

This error wasn't detected before, because both rmw_fastrtps_dynamic_cpp and rmw_cyclonedds_cpp were doing "weird stuff" instead of using the "get function" and "resize function" provided by the introspection typesupport.
For further reference, see:

• Memory leak about not to call placement delete after calling placement new rmw_cyclonedds#219
• [rmw_fastrtps_dynamic_cpp] Typesupport implementation leaks and it relies in conceptually broken cpp rmw_fastrtps#428

[dirk-thomas]

The following PRs must be merged simultaneously:

How about Connext? If it doesn't need the change is its current behavior broken?

This error wasn't detected before, because both rmw_fastrtps_dynamic_cpp and rmw_cyclonedds_cpp were doing "weird stuff" instead of using the "get function" and "resize function" provided by the introspection typesupport.

Both were using the provided function. And the "weird stuff" was simply accounting the difference in the API - to exactly pass what was needed.

Certainly unifying the signature makes sense.

[dirk-thomas]

The following PRs must be merged simultaneously:

These changes will also need to be released into Rolling.

[ivanpauno]

How about Connext? If it doesn't need the change is its current behavior broken?

rmw_connexy_dynamic_cpp is not supported, and the code was not handling the semantic difference correctly https://github.com/ros2/rmw_connext/blob/c0a12cd0c6b8a14191763417e2ee13bbd9624492/rmw_connext_dynamic_cpp/src/templates.hpp#L1271.

Both were using the provided function. And the "weird stuff" was simply accounting the difference in the API - to exactly pass what was needed.

They weren't using those functions, that was fixed in ros2/rmw_cyclonedds#228, ros2/rmw_fastrtps#429. The code handling the introspection typesupport before those PRs is interesting.

These changes will also need to be released into Rolling.

👍

[dirk-thomas]

rmw_connexy_dynamic_cpp is not supported, and the code was not handling the semantic difference correctly https://github.com/ros2/rmw_connext/blob/c0a12cd0c6b8a14191763417e2ee13bbd9624492/rmw_connext_dynamic_cpp/src/templates.hpp#L1271.

I would suggest to still update the code (since it is trivial to do now) and probably hard to figure out that it is needed in the future.

[ivanpauno]

I would suggest to still update the code (since it is trivial to do now) and probably hard to figure out that it is needed in the future.

Yeah, what I mean is that rmw_connext_dynamic_cpp doesn't need a patch. That code was always using:

member->get_function(ros_message + member->offset_, index)

[dirk-thomas]

Yeah, what I mean is that rmw_connext_dynamic_cpp doesn't need a patch. That code was always using:

Oh, got it now. Thanks for clarifying. So the behavior must have been broken in the past then. ✅

[ivanpauno]

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

rosdistro PRs:

• rosidl: 2.0.0-1 in 'rolling/distribution.yaml' [bloom] ros/rosdistro#26688
• rmw_cyclonedds: 0.18.1-1 in 'rolling/distribution.yaml' [bloom] ros/rosdistro#26689
• rmw_fastrtps: 3.1.1-1 in 'rolling/distribution.yaml' [bloom] ros/rosdistro#26690