change generators to IDL-based pipeline #14
Conversation
dirk-thomas
added
in review
dirk-thomas
changed the title
rosidl_typesupport_fastrtps_c/resource/idl__rosidl_typesupport_fastrtps_c.h.em
Outdated
Show resolved
Hide resolved
rosidl_typesupport_fastrtps_c/resource/idl__type_support_c.cpp.em
Outdated
Show resolved
Hide resolved
| @[ else]@ | ||
| if (!@(field.type.pkg_name)__msg__@(field.type.type)__callbacks->cdr_serialize( | ||
| &ros_message->@(field.name), cdr)) | ||
| if (!callbacks->cdr_serialize( |
There was a problem hiding this comment.
what if member.type is a WString instance?
There was a problem hiding this comment.
WString is not yet supported throughout all generators.
There was a problem hiding this comment.
Does that mean that member.type won't ever be a WString instance? If so, disregard my comment. Otherwise an informative error would be nice.
| cdr.serializeArray(array_ptr, size); | ||
| @[ else]@ | ||
| for (size_t i = 0; i < size; ++i) { | ||
| if (!@(field.type.pkg_name)__msg__@(field.type.type)__callbacks->cdr_serialize( | ||
| if (!callbacks->cdr_serialize( |
There was a problem hiding this comment.
what if member.type.basetype is a WString instance?
There was a problem hiding this comment.
Still applies, it'll try to use callbacks which will be undefined.
| (void)array_ptr; | ||
| size_t item_size = sizeof(array_ptr[0]); | ||
| current_alignment += array_size * item_size + | ||
| eprosima::fastcdr::Cdr::alignment(current_alignment, item_size); | ||
| @[ else] | ||
| for (size_t index = 0; index < array_size; ++index) { | ||
| current_alignment += get_serialized_size_@(field.type.pkg_name)__msg__@(field.type.type)( | ||
| current_alignment += get_serialized_size_@('__'.join(member.type.basetype.namespaces + [member.type.basetype.name]))( |
There was a problem hiding this comment.
what if member.type.basetype is a WString instance?
| current_alignment += item_size + | ||
| eprosima::fastcdr::Cdr::alignment(current_alignment, item_size); | ||
| } | ||
| @[ else] | ||
| current_alignment += get_serialized_size_@(field.type.pkg_name)__msg__@(field.type.type)( | ||
| &(ros_message->@(field.name)), current_alignment); | ||
| current_alignment += get_serialized_size_@('__'.join(member.type.namespaces + [member.type.name]))( |
There was a problem hiding this comment.
what if member.type is a WString instance?
| if isinstance(type_, NestedType): | ||
| type_ = type_.basetype | ||
| }@ | ||
| @[ if isinstance(type_, String)]@ |
| "${_srv_include_dir}" | ||
| "${_action_include_dir}" | ||
| ) | ||
| # TODO(sloretz) parent_folder/dds_fastrtps in template instead of here |
There was a problem hiding this comment.
should we do something about this TODO? do we have to?
There was a problem hiding this comment.
@sloretz Can you clarify what this is about?
There was a problem hiding this comment.
@dirk-thomas I don't remember. If I had to guess, I'd assume this was to make the template includes look like #include <msg/dds_fastrtps/something.hpp> instead of adding msg/dds_fastrtps to the target's include directories. If that's what this means, then I have no idea which template or includes this is talking about.
There was a problem hiding this comment.
I removed the comment in ef4fe78.
| } | ||
| @[ end if]@ | ||
| } | ||
| @[ else]@ | ||
| @[ if field.type.type == 'string']@ | ||
| @[ if isinstance(member.type, String)]@ |
There was a problem hiding this comment.
same questions about WString (maybe use an assert?)
| if isinstance(type_, NestedType): | ||
| type_ = type_.basetype | ||
| }@ | ||
| @[ if isinstance(type_, String)]@ |
dirk-thomas
force-pushed
the
idl-stage-7
branch
from
c4e2e0d to
39743a5
Compare
dirk-thomas
added
in progress
dirk-thomas
changed the title
dirk-thomas
added
in review
There was a problem hiding this comment.
LGTM overall, but I'm a bit wary about WString as it's handled in some places but not all.
I understand that we're not looking forward to support it in the near term, but then IMHO an explanatory error message sounds better than an obscure C++ compilation error coming from generated code. Just my 2 cents.
Connected to ros2/rosidl#334.