Refactor get topic names and types #110
Conversation
faa6768 to
afe8c40
Compare
| rmw_ret_t | ||
| rmw_get_service_names_and_types( | ||
| const rmw_node_t * node, | ||
| rcutils_allocator_t * allocator, |
There was a problem hiding this comment.
nitpick: could that be a const pointer?
There was a problem hiding this comment.
I'm not 100% sure. On the one hand the allocator's state is mutable (i.e. when someone calls .allocate it might change the internal state of the allocator), on the other the const in the strict functional sense (see: https://stackoverflow.com/questions/13181546/const-correctness-for-structs-with-pointers) will only restrict changing the function pointers in the allocator.
Here I was using non-const to indicate the allocator is not just "read" from, but instead it is "used" which in my mind is something like "written to".
In short, it could be but the rest of our code which takes an allocator pointer is already non-const to indicate the allocator might be mutated in the function.
If we can find reason is should be const, then I'd update all of them at once rather than just this function.
| RMW_PUBLIC | ||
| RMW_WARN_UNUSED | ||
| rmw_ret_t | ||
| rmw_names_and_types_fini(rmw_names_and_types_t * names_and_types); |
There was a problem hiding this comment.
shouldn't that take an allocator as well?
| // Use the allocator in the names string array | ||
| // (prevents this data structure from having to also store it) | ||
| names_and_types->names.allocator.deallocate( | ||
| names_and_types->types, names_and_types->names.allocator.state); |
There was a problem hiding this comment.
can there be a mismatch between this allocator and the allocator used for the init function?
| names_and_types->types = NULL; | ||
| } | ||
| // Cleanup names string array | ||
| rcutils_ret = rcutils_string_array_fini(&names_and_types->names); |
There was a problem hiding this comment.
doesn't this call take an allocator as well?
https://github.com/ros2/rcutils/blob/master/include/rcutils/types/string_array.h#L86
Connects to ros2/ros2#361