Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport #2

methylDragon · 2023-04-07T08:07:29Z

See: ros2/ros2#1405

wjwwood

A few comments, otherwise lgtm

include/rosidl_dynamic_typesupport/dynamic_message_type_support_struct.h

Signed-off-by: methylDragon <methylDragon@gmail.com>

wjwwood

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.

wjwwood · 2023-04-10T17:12:14Z

include/rosidl_dynamic_typesupport/api/dynamic_data.h

 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);


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.

emersonknapp · 2023-04-10T18:03:58Z

include/rosidl_dynamic_typesupport/api/dynamic_data.h

+struct rosidl_dynamic_typesupport_dynamic_data_s
+{
+  rcutils_allocator_t allocator;
+  rosidl_dynamic_typesupport_dynamic_data_impl_t impl;


Not using hidden-type pimpl?

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

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?

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

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.

The implementation contains a void * handle, so that's a good escape hatch I think?

Yeah, it's good enough for now. It isn't our usual style, but it will do in a pinch.

emersonknapp · 2023-04-10T18:07:01Z

src/api/dynamic_data.c

 {
  RCUTILS_CHECK_ARGUMENT_FOR_NULL(dynamic_data, RCUTILS_RET_INVALID_ARGUMENT);
+  RCUTILS_CHECK_ARGUMENT_FOR_NULL(allocator, RCUTILS_RET_INVALID_ARGUMENT);


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.

I might defer this so we don't invalidate the windows debug CI run :x

(I fixed it)

clalancette

The general idea seems OK to me, but I think we are missing a PR here (or I'm misunderstanding something).

src/api/dynamic_data.c

Signed-off-by: methylDragon <methylDragon@gmail.com>

clalancette · 2023-04-10T20:38:26Z

include/rosidl_dynamic_typesupport/api/dynamic_data.h

+struct rosidl_dynamic_typesupport_dynamic_data_s
+{
+  rcutils_allocator_t allocator;
+  rosidl_dynamic_typesupport_dynamic_data_impl_t impl;


Yeah, it's good enough for now. It isn't our usual style, but it will do in a pinch.

methylDragon force-pushed the runtime_interface_reflection_allocators branch from 061eddf to b988c8d Compare April 7, 2023 10:19

methylDragon mentioned this pull request Apr 7, 2023

Dynamic Subscription (REP-2011 subset): BONUS: Allocator Refactor ros2/ros2#1405

Open

28 tasks

methylDragon force-pushed the runtime_interface_reflection_allocators branch from b988c8d to 919f881 Compare April 7, 2023 21:35

methylDragon closed this Apr 9, 2023

methylDragon force-pushed the runtime_interface_reflection_allocators branch from 919f881 to de367f3 Compare April 9, 2023 06:48

methylDragon reopened this Apr 9, 2023

wjwwood approved these changes Apr 10, 2023

View reviewed changes

methylDragon force-pushed the runtime_interface_reflection_allocators branch from f338b56 to 8c78215 Compare April 10, 2023 04:22

Refactor dynamic message type support impl to use allocators

69d2518

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon force-pushed the runtime_interface_reflection_allocators branch from 8c78215 to 7c19712 Compare April 10, 2023 05:36

Refactor serialization support to use allocators and refs

7d0d91b

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon force-pushed the runtime_interface_reflection_allocators branch from 7c19712 to 7d0d91b Compare April 10, 2023 05:39

Refactor dynamic type support structs to use allocators and refs

7693180

Signed-off-by: methylDragon <methylDragon@gmail.com>

wjwwood approved these changes Apr 10, 2023

View reviewed changes

emersonknapp reviewed Apr 10, 2023

View reviewed changes

clalancette requested changes Apr 10, 2023

View reviewed changes

src/api/dynamic_data.c Show resolved Hide resolved

Check if allocator is valid

efcac65

Signed-off-by: methylDragon <methylDragon@gmail.com>

methylDragon requested a review from clalancette April 10, 2023 20:27

clalancette approved these changes Apr 10, 2023

View reviewed changes

methylDragon merged commit 8861cdc into rolling Apr 11, 2023

delete-merged-branch bot deleted the runtime_interface_reflection_allocators branch April 11, 2023 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport #2

Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport #2

methylDragon commented Apr 7, 2023

wjwwood left a comment •

edited

Loading

wjwwood left a comment

wjwwood Apr 10, 2023

emersonknapp Apr 10, 2023

methylDragon Apr 10, 2023

clalancette Apr 10, 2023

methylDragon Apr 10, 2023

clalancette Apr 10, 2023

methylDragon Apr 10, 2023

clalancette Apr 10, 2023

emersonknapp Apr 10, 2023

methylDragon Apr 10, 2023 •

edited

Loading

clalancette left a comment

clalancette Apr 10, 2023

Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport #2

Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport #2

Conversation

methylDragon commented Apr 7, 2023

wjwwood left a comment • edited Loading

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methylDragon Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment •

edited

Loading

methylDragon Apr 10, 2023 •

edited

Loading