use string_array_t from c_utilities package #100
Conversation
Karsten1987
force-pushed
the
utilities_string_array
branch
from
f1a79b3
to
ba4b4fc
Compare
| @@ -336,13 +338,7 @@ RMW_WARN_UNUSED | |||
| rmw_ret_t | |||
| rmw_get_node_names( | |||
| const rmw_node_t * node, | |||
| rmw_string_array_t * node_names); | |||
|
|
|||
| RMW_PUBLIC | |||
There was a problem hiding this comment.
Why are you removing this function instead of just changing the type?
There was a problem hiding this comment.
the destroy function is part of the c_utilities package, here:
https://github.com/ros2/c_utilities/blob/master/include/c_utilities/types/string_array.h#L77
mikaelarguedas
changed the title
rmw/CMakeLists.txt
Outdated
| configure_rmw_library(${PROJECT_NAME} LANGUAGE "C") | ||
|
|
||
| ament_export_dependencies(c_utilities) |
There was a problem hiding this comment.
This line could be merged with the next.
|
Outsider's view here, so feel free to take it with a grain of salt, as I likely don't see the larger picture. It seems that this PR adds an additional dependency to every |
Yes, pretty much. |
|
Gotcha - I missed the corresponding issue #94 |
|
I had the first initial thought, given that I touched basically the complete stack of packages, however given that it's "simply" only utility functions, which itselves don't have any dependencies and stay a clean c-package, I think it's a reasonable trade-off. |
|
Linux: |
Karsten1987
added
in review
|
I think that's up for review! |
This is makes a nicer diff in the future if the list is appended.
|
linux: |
|
CI's passing. I am going to merge this (and related PR) soonish if nobody has anything comments anymore. |
|
given the changes in Linux: |
I remove the
rmw_string_array_tin lieu of the implementation inc_utilities.Connected PRs:
c_utilities: ros2/rcutils#14
rmw_fastrtps: ros2/rmw_fastrtps#102
rmw_connext: ros2/rmw_connext#224
rmw_impl: ros2/rmw_implementation#20
rcl: ros2/rcl#118
rclcpp: ros2/rclcpp#320
rclpy: ros2/rclpy#75