Dynamic Subscription (BONUS: Allocators): rosidl_dynamic_typesupport_fastrtps #3
Conversation
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
methylDragon
force-pushed
the
runtime_interface_reflection_allocators
branch
from
774addf
to
6c9c5b2
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
methylDragon
force-pushed
the
runtime_interface_reflection_allocators
branch
from
6c9c5b2
to
f5f1b30
Compare
| @@ -444,7 +449,7 @@ fastrtps__dynamic_data_get_string_value( | |||
| ); | |||
|
|
|||
| *value_length = tmp_string.size(); | |||
| char * tmp_out = new char[*value_length + 1]; | |||
| char * tmp_out = new char[*value_length + 1]; // TODO(methylDragon): Use alloc here | |||
There was a problem hiding this comment.
I'm not sure which alloc you are referring to here. Can you be more specific? I would think we would need to pass in an allocator and use that, but I'm not sure.
Further, we need to check for allocation failure here. Either we need to specify nothrow and check for NULL, or capture the exception (which won't be able to bubble through the C layer that calls into this).
Same goes a few times below.
There was a problem hiding this comment.
Just the new, yeah.. Because the caller will be in charge of destroying the obtained buffer
I was thinking it'd be good if they did it using the allocator stored in the data_impl, but because it was an implementation detail, I didn't want to get to it yet
There was a problem hiding this comment.
This one I will defer for now (hence the TODO)
| *type_builder_impl = new rosidl_dynamic_typesupport_dynamic_type_builder_impl_t{ | ||
| std::move(type_builder_handle)}; | ||
|
|
||
| type_builder_impl->handle = std::move(type_builder_handle); |
There was a problem hiding this comment.
I don't think the std::move does anything here, right? These are both just pointers. For that matter, you could probably just do type_builder_impl->handle = fastrtps_impl->type_factory_->create_struct_builder(); above, though maybe C++ would warn about assigning a DynamicTypeBuilder * to a void *, I'm not sure.
Same question a few times below.
There was a problem hiding this comment.
Yeah, that's true.. I just thought it was more explicit, stylistically, but I can just do an assignment without the move?
There was a problem hiding this comment.
Yeah, that's true.. I just thought it was more explicit, stylistically, but I can just do an assignment without the move?
When I see a std::move, I'm looking for the transfer of ownership, which I guess is happening in principle, but not by the C++ rules. For a pointer assignment, I think just the direct assignment would be better, yeah.
There was a problem hiding this comment.
Okay, I will edit
Edit: Should we defer this till after the merge?
delete-merged-branch
bot
deleted the
runtime_interface_reflection_allocators
branch
See: ros2/ros2#1405