ros2 / rosidl

Packages which provide the ROS IDL (.msg) definition and code generation.
Apache License 2.0
76 stars 125 forks source link

Inconsistent APIs between String and U16String #764

Closed allsey87 closed 3 months ago

allsey87 commented 1 year ago

Bug report

rosidl_runtime_c__U16String__resize exists, however, there is no rosidl_runtime_c__String__resize.

Required Info:

Expected behavior

Consistent APIs between String and U16String

Actual behavior

Inconsistent APIs between String and U16String

Feature request

Make APIs consistent?

Feature description

Sometimes a char * is not directly available, e.g., from a deserialization library. This means that it is not possible to use assignn without allocating an intermediate buffer. Having a resize function for both String and U16String would solve this problem.

Implementation considerations

The workaround I will use is to use rcutils_get_default_allocator to update the internals of the string manually.

sloretz commented 1 year ago

Thank you for the report! It looks like a resize function is missing. There's a resize_assignn test, but it doesn't resize an existing string. I'll leave this open as a help wanted. Fixing it means doing the following:

Make a declaration similar to this one, but using char instead of uint16_t and put it in string_functions.h

https://github.com/ros2/rosidl/blob/d66b6db0cb93963bdf11b3b315d95fd5fc5f0ef5/rosidl_runtime_c/include/rosidl_runtime_c/u16string_functions.h#L156-L167

Implement it similar to this function, but using char instead of uint16_t and put it in string_functions.c

https://github.com/ros2/rosidl/blob/d66b6db0cb93963bdf11b3b315d95fd5fc5f0ef5/rosidl_runtime_c/src/u16string_functions.c#L167-L188

Take the expectations on U16String_resize from the test below, and put them into into test_string_functions.cpp for the new function.

https://github.com/ros2/rosidl/blob/d66b6db0cb93963bdf11b3b315d95fd5fc5f0ef5/rosidl_runtime_c/test/test_u16string_functions.cpp#L73-L147

r7vme commented 3 months ago

Seems this issue is resolved by https://github.com/ros2/rosidl/pull/806 . @allsey87 or @sloretz could you please close this issue?