Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport#2
Conversation
061eddf to
b988c8d
Compare
b988c8d to
919f881
Compare
919f881 to
de367f3
Compare
f338b56 to
8c78215
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
8c78215 to
7c19712
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
7c19712 to
7d0d91b
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
wjwwood
left a comment
There was a problem hiding this comment.
lgtm, I started reviewing this last night and finished it this morning. Though there are a lot of lines changed, they are just repeating for the most part.
| ROSIDL_DYNAMIC_TYPESUPPORT_PUBLIC | ||
| rcutils_ret_t | ||
| rosidl_dynamic_typesupport_dynamic_data_clone( | ||
| const rosidl_dynamic_typesupport_dynamic_data_t * other_dynamic_data, | ||
| rosidl_dynamic_typesupport_dynamic_data_t ** dynamic_data); // OUT | ||
| rcutils_allocator_t * allocator, | ||
| rosidl_dynamic_typesupport_dynamic_data_t * dynamic_data); // OUT | ||
|
|
||
| ROSIDL_DYNAMIC_TYPESUPPORT_PUBLIC | ||
| rcutils_ret_t | ||
| rosidl_dynamic_typesupport_dynamic_data_fini( | ||
| rosidl_dynamic_typesupport_dynamic_data_t * dynamic_data); |
There was a problem hiding this comment.
For other reviewers, the vast majority of changes for this pr can be summed up with this diff, basically adding allocators where we do allocation and avoiding double pointers as out parameters (matching how most of the rcl-like interfaces work, and the rest is mostly mechanical changes related to this shift.
| struct rosidl_dynamic_typesupport_dynamic_data_s | ||
| { | ||
| rcutils_allocator_t allocator; | ||
| rosidl_dynamic_typesupport_dynamic_data_impl_t impl; |
There was a problem hiding this comment.
I was thinking of doing that, but I thought it would be better to enforce that the impl is a complete type (and then defer it to the handle), so that we can enforce that the impl type has an allocator
There was a problem hiding this comment.
But isn't the the type defined just above? That is, this implementation is completely in control of that structure, and thus can guarantee that it always has an allocator, no?
There was a problem hiding this comment.
ah, right.. I think I'm avoiding the use of a pointer so we don't have to keep doing null checks on the impl member as well..
The impl is unusable without the associated serialization support in either case, so in my view composition makes sense
There was a problem hiding this comment.
Yeah, actually, I largely agree with you. The only problem here is that if we want to add, remove, or change structure members in the implementation, we won't be able to backport it due to ABI concerns. But if we are OK with that limitation for now, we can leave it as-is.
There was a problem hiding this comment.
The implementation contains a void * handle, so that's a good escape hatch I think?
There was a problem hiding this comment.
Yeah, it's good enough for now. It isn't our usual style, but it will do in a pinch.
| rosidl_dynamic_typesupport_dynamic_data_t * loaned_dynamic_data) | ||
| { | ||
| RCUTILS_CHECK_ARGUMENT_FOR_NULL(dynamic_data, RCUTILS_RET_INVALID_ARGUMENT); | ||
| RCUTILS_CHECK_ARGUMENT_FOR_NULL(allocator, RCUTILS_RET_INVALID_ARGUMENT); |
There was a problem hiding this comment.
I think you need to check rcutils_allocator_is_valid(allocator) as part of argument checking (for all) - allocator could be non-null but contain null function pointers.
There was a problem hiding this comment.
I might defer this so we don't invalidate the windows debug CI run :x
(I fixed it)
clalancette
left a comment
There was a problem hiding this comment.
The general idea seems OK to me, but I think we are missing a PR here (or I'm misunderstanding something).
Signed-off-by: methylDragon <methylDragon@gmail.com>
| struct rosidl_dynamic_typesupport_dynamic_data_s | ||
| { | ||
| rcutils_allocator_t allocator; | ||
| rosidl_dynamic_typesupport_dynamic_data_impl_t impl; |
There was a problem hiding this comment.
Yeah, it's good enough for now. It isn't our usual style, but it will do in a pinch.
See: ros2/ros2#1405